MINOR: trivial cleanups, javadoc errors, omitted StateStore tests, etc. by dongjinleekr · Pull Request #8130 · apache/kafka

dongjinleekr · 2020-02-18T05:58:18Z

Add omitted [WindowStoreBuilderTest, TimestampedWindowStoreBuilderTest]#shouldThrowNullPointerIfMetricsScopeIsNull: Other StateStores has it.
Improve Stores Javadoc
Remove unused method + duplicated parameters
Remove duplicated spaces
Remove unthrown Exceptions + align javadoc parameters
Remove unnecessary public from [Sink,Source]TaskContext
Add omitted WindowStore, SessionStore test cases: GlobalStateStoreProviderTest, StreamThreadStateStoreProviderTest (Compare it with KeyValueStore counterpart cases.)
Fix MeteredTimestampedWindowStore javadoc

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

dongjinleekr · 2020-02-19T01:17:17Z

Retest this please.

dongjinleekr · 2020-02-25T05:30:26Z

Rebased onto the latest trunk & remove one more unused import. cc/ @hachikuji @omkreddy

dongjinleekr · 2020-04-01T15:39:29Z

Rebased onto the latest trunk, with removing duplicated entity in checkstyle/import-control.xml.

omkreddy · 2020-04-04T14:24:29Z

Fixed! 😃

omkreddy · 2020-04-04T14:35:08Z

retest this please

omkreddy · 2020-04-04T14:37:26Z

cc @mjsax @vvcephei for streams test changes

mjsax · 2020-04-04T17:11:27Z

Just to clarify. To put two blanks between two sentences is a writing style that goes back to classic type writers -- having a larger space between sentences makes it easier to read. Hence, removing all those double-blanks (to correct writing style) is just a lot of "noise" with no real value...

We can still merge this PR (as you put a lot of work into it already), however, I would encourage you to not do such PRs in the future. It's just additional review load (and committers already cannot handle the current load...) with no real value and thus a "waste" of your own time as well as the reviewer's time.

@mjsax I see. I’ll be more careful from now on. Thanks for the detailed explanation.

mjsax · 2020-04-04T17:13:42Z

This is public API and cannot just be removed. Why do you think it should be removed?

I'm sorry, it was a mistake; So reverted.

@dongjinleekr Seems you did not revert this change yet.

mjsax · 2020-04-04T17:19:45Z

Where do we pass a null value? Also, where is the MetricScope in this test?

supplier#metricsScope() returns null, for its setUp() configuration. It is same to [KeyValueStoreBuilderTest, TimestampedKeyValueStoreBuilderTest]#shouldThrowNullPointerIfMetricsScopeIsNull. SessionStoreBuilderTest#shouldThrowNullPointerIfMetricsScopeIsNull does same, but it also validates Exception message. (Wait, is this way more recommended?)

Thanks for the clarification. However, I am still not sure if I understand this test. The constructor TimestampedWindowStoreBuilder does not call supplier.metricsScope() -- hence, it am still puzzled why this test would throw a NPE? To test metricScope() we would need to call build() method. In fact, following the call trace, we would never check if metricScope() returns null or not. Seems worth to fix... (maybe add a check in MeteredWindowStore constructor? -- not sure if the other MeteredXxxStore have a similar check. If not, would be good to add one.)

Seems it comes from supplier.name() -- for this case, the test setup is incorrect -- also in the test above actually, that would also fail with supplier.name() NPE, but not with the NPE they want to test.

does same, but it also validates Exception message. (Wait, is this way more recommended?)

Yes, it's always recommended to verify the exception message to make sure we test the right think. Using (expected = NullPointerException.class) annotation is not ideal.

@mjsax I reviewed the problem more carefully and concluded that you are right - the NullPointerException here is not related to MetricScope. We need to rewrite the series of tests.

Let's review TimestampedWindowStoreBuilderTest#shouldThrowNullPointerIfMetricsScopeIsNull as an example. Here, supplier#name is null so when instantiating TimestampedWindowStoreBuilder, it fails in its parent class's constructor, AbstractStoreBuilder line 41 - Objects.requireNonNull(name, "name cannot be null");. It is also why SessionStoreBuilderTest#shouldThrowNullPointerIfMetricsScopeIsNull ends with a error message "name cannot be null" - obviously, it also has nothing to do with MetricScope.

Here is the debugger status; As you can see, supplier#name is null here.

I am now rewriting the tests. Stay tuned! 😄

Thanks! Highly appreciated!

mjsax · 2020-04-04T17:20:16Z

Same question as above?

Same answer as above. 😓

mjsax · 2020-04-04T17:23:16Z

The build is currently broker. There is already a PR to fix it: #8424

No need to retest until trunk is fixed.

mjsax · 2020-05-27T19:21:07Z

@dongjinleekr -- this PR needs a rebase. Might be nice to get it into 2.6?

dongjinleekr · 2020-06-20T13:01:11Z

@mjsax Here is the fix. I investigated the problem and found the following.

The root of the problem is: the consturctors of XXXStoreBuilder classes omit the MetricsScope nullity check (see commit 5b432d9.) As of present, the NullPointerExceptions are thrown from the constructor of AbstractStoreBuilder - it has nothing to do with MetricsScope. (Interestingly, SessionStoreBuilderTest#shouldThrowNullPointerIfMetricsScopeIsNull already takes the right way.)

As a evidence, if we give the XXXBytesStoreSupplier mock with non-null name no exception is thrown. (see commit 01f2cf3)

To fix this problem, I added a validation method to each of the constructor of XXXStoreBuilder; To make the tests not broken, I also made the supplier mocks to return non-null MetricsScope by expect(supplier.metricsScope()).andReturn("metricScope"); (see commit 2e0043b)

+1. I removed the 'two space' removal while rebasing onto the latest trunk; You are right, it is worthless. Instead, I added the other trivial fixes I found during reading the code.

dongjinleekr · 2020-09-10T14:03:05Z

@mjsax Could this PR get it into 2.7? 😃

mjsax · 2020-09-30T18:35:48Z

@guozhangwang Is this ok? I am not familiar with this part of the code. If false the default anyway?

LGTM, since the method definition also has isRecoveredSwapFile: Boolean = false by default.

Thanks @guozhangwang for verifying.

mjsax

Overall LGTM.

I am not familiar with core code so I hope @guozhangwang helps out. One comment though that needs to be resolved.

mjsax · 2020-09-30T18:36:14Z

@dongjinleekr Seems you did not revert this change yet.

mjsax · 2020-09-30T18:43:01Z

It also seems that some test failed due to NPE. Those seems to need updates.

…t]#shouldThrowNullPointerIfMetricsScopeIsNull: Other StateStores has it.

1. Fix javadoc typo: Stores#timestampedWindowStoreBuilder, windows-store -> window-store (Compare with Stores#timestampedKeyValueStoreBuilder) 2. Add omitted IllegalArgumentException condition in Stores#lruMap. 3. Add omitted IllegalArgumentException condition in WindowBytesStoreSupplier methods.

1. Remove unused method: TimeWindowedSerde#timeWindowedSerdeFrom(Class, long) 2. Remove duplicated parameter: Log#splitOverflowedSegment

…viderTest, StreamThreadStateStoreProviderTest (Note: compare with KeyValueStore cases.)

…re]BuilderTest#shouldThrowNullPointerIfMetricsScopeIsNull: now checks Exception message.

…ue,WindowStore,TimestampedWindowStore]BuilderTest#shouldThrowNullPointerIfMetricsScopeIsNull; No exceptions are thrown now.

…w,TimestampedWindow,Session]StoreBuilder constructors

…th 'TimestampedWindowStoreBuilder', 'SessionStoreBuilder', etc.)

- `KeyValueStoreBuilder`: `bytesStoreSupplier` → `storeSupplier` - `TimestampedKeyValueStoreBuilder`: `bytesStoreSupplier` → `storeSupplier` - `TimestampedWindowStoreBuilder`: `bytesStoreSupplier` → `storeSupplier` - `SessionStoreBuilder`: `supplier` → `storeSupplier`

…ppressScenarioTest

…shouldCreateKeyValueStoreWithTheProvidedInnerStore to return metricScope: without it, NullPointerExcepion is thrown during TimestampedKeyValueStore instance creation.

dongjinleekr · 2020-10-07T16:04:49Z

@mjsax The failing test is now fixed with commit 9f7a73f and mistakenly removed method is reverted in commit 6972309. Also, rebased onto the latest trunk.
@guozhangwang Thanks for reviewing the code! 😃

mjsax · 2020-10-08T02:08:46Z

Thanks for the PR @dongjinleekr! Merged to trunk.

…c. (apache#8130) Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Guozhang Wang <guozhang@confluent.io>, Matthias J. Sax <matthias@confluent.io>

* commit '2804257fe221f37e5098bd': (67 commits) KAFKA-10562: Properly invoke new StateStoreContext init (apache#9388) MINOR: trivial cleanups, javadoc errors, omitted StateStore tests, etc. (apache#8130) KAFKA-10564: only process non-empty task directories when internally cleaning obsolete state stores (apache#9373) KAFKA-9274: fix incorrect default value for `task.timeout.ms` config (apache#9385) KAFKA-10362: When resuming Streams active task with EOS, the checkpoint file is deleted (apache#9247) KAFKA-10028: Implement write path for feature versioning system (KIP-584) (apache#9001) KAFKA-10402: Upgrade system tests to python3 (apache#9196) KAFKA-10186; Abort transaction with pending data with TransactionAbortedException (apache#9280) MINOR: Remove `TargetVoters` from `DescribeQuorum` (apache#9376) Revert "KAFKA-10469: Resolve logger levels hierarchically (apache#9266)" MINOR: Don't publish javadocs for raft module (apache#9336) KAFKA-9929: fix: add missing default implementations (apache#9321) KAFKA-10188: Prevent SinkTask::preCommit from being called after SinkTask::stop (apache#8910) KAFKA-10338; Support PEM format for SSL key and trust stores (KIP-651) (apache#9345) KAFKA-10527; Voters should not reinitialize as leader in same epoch (apache#9348) MINOR: Refactor unit tests around RocksDBConfigSetter (apache#9358) KAFKA-6733: Printing additional ConsumerRecord fields in DefaultMessageFormatter (apache#9099) MINOR: Annotate test BlockingConnectorTest as integration test (apache#9379) MINOR: Fix failing test due to KAFKA-10556 PR (apache#9372) KAFKA-10439: Connect's Values to parse BigInteger as Decimal with zero scale. (apache#9320) ...

dongjinleekr force-pushed the feature/cleanup/202002 branch from 4647690 to 2fd4010 Compare February 25, 2020 05:29

dongjinleekr force-pushed the feature/cleanup/202002 branch 2 times, most recently from b05cd4a to 68d3f13 Compare March 2, 2020 04:42

dongjinleekr force-pushed the feature/cleanup/202002 branch from 68d3f13 to 99fc213 Compare April 1, 2020 15:38

dongjinleekr mentioned this pull request Apr 1, 2020

KAFKA-9366: Change log4j dependency into log4j2 #7898

Closed

3 tasks

omkreddy reviewed Apr 4, 2020

View reviewed changes

mjsax reviewed Apr 4, 2020

View reviewed changes

dongjinleekr force-pushed the feature/cleanup/202002 branch from 7077050 to 24b2515 Compare June 20, 2020 12:58

dongjinleekr force-pushed the feature/cleanup/202002 branch from 24b2515 to 382cbcb Compare August 13, 2020 07:15

dongjinleekr force-pushed the feature/cleanup/202002 branch from 382cbcb to 60ae960 Compare September 10, 2020 13:57

mjsax reviewed Sep 30, 2020

View reviewed changes

dongjinleekr added 5 commits October 8, 2020 00:47

Add omitted [WindowStoreBuilderTest, TimestampedWindowStoreBuilderTes…

7991605

…t]#shouldThrowNullPointerIfMetricsScopeIsNull: Other StateStores has it.

Remove unused method + duplicated parameters

3f9f171

1. Remove unused method: TimeWindowedSerde#timeWindowedSerdeFrom(Class, long) 2. Remove duplicated parameter: Log#splitOverflowedSegment

Remove unthrown Exceptions: TaskManager, ProduceBenchWorker

9bdc180

Remove unnecessary public from [Sink,Source]TaskContext

bede35c

dongjinleekr added 13 commits October 8, 2020 00:47

Fix incorrect MeteredTimestampedWindowStore javadoc

1f89531

Remove duplicated entity in checkstyle/import-control.xml

b9525c5

Add omitted WindowStore, SessionStore test cases: GlobalStateStorePro…

ada16b4

…viderTest, StreamThreadStateStoreProviderTest (Note: compare with KeyValueStore cases.)

Update [KeyValue,TimestampedKeyValue,WindowStore,TimestampedWindowSto…

d581d15

…re]BuilderTest#shouldThrowNullPointerIfMetricsScopeIsNull: now checks Exception message.

Give BytesStoreSupplier mock with name to [KeyValue,TimestampedKeyVal…

fc7194d

…ue,WindowStore,TimestampedWindowStore]BuilderTest#shouldThrowNullPointerIfMetricsScopeIsNull; No exceptions are thrown now.

Add metricsScope nullity check to [KeyValue,TimestampedKeyValue,Windo…

df01c35

…w,TimestampedWindow,Session]StoreBuilder constructors

Add nullity check in 'WindowStoreBuilder' constructor. (For parity wi…

29d766a

…th 'TimestampedWindowStoreBuilder', 'SessionStoreBuilder', etc.)

Remove unthrown Exception in SessionStoreBuilderTest#setUp

ac6ac58

Remove unused variables: RocksDBStore, SuppressionIntegrationTest

dff84f2

Remove redundant type parameters from AbstractJoinIntegrationTest, Su…

e92dda5

…ppressScenarioTest

Revert WindowedSerdes#timeWindowedSerdeFrom(Class, long)

6972309

Add supplier mock object in TimestampedKeyValueStoreMaterializerTest#…

9f7a73f

…shouldCreateKeyValueStoreWithTheProvidedInnerStore to return metricScope: without it, NullPointerExcepion is thrown during TimestampedKeyValueStore instance creation.

dongjinleekr force-pushed the feature/cleanup/202002 branch from 60ae960 to 9f7a73f Compare October 7, 2020 16:02

mjsax merged commit 8d4bbf2 into apache:trunk Oct 8, 2020

Conversation

dongjinleekr commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

dongjinleekr commented Feb 19, 2020

Uh oh!

dongjinleekr commented Feb 25, 2020

Uh oh!

dongjinleekr commented Apr 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkreddy commented Apr 4, 2020

Uh oh!

omkreddy commented Apr 4, 2020

Uh oh!

mjsax Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Apr 4, 2020

Uh oh!

mjsax commented May 27, 2020

Uh oh!

dongjinleekr commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjinleekr commented Sep 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Sep 30, 2020

Uh oh!

dongjinleekr commented Oct 7, 2020

Uh oh!

mjsax commented Oct 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dongjinleekr commented Feb 18, 2020 •

edited

Loading

mjsax Apr 4, 2020 •

edited

Loading

dongjinleekr commented Jun 20, 2020 •

edited

Loading