HOTFIX: Fix wrong setting of Serde in MeteredTimestampWindowStore#6808
Conversation
| } | ||
|
|
||
| @Test | ||
| public void shouldNotThrowExceptionIfSerdesCorrectlySetFromProcessorContext() { |
There was a problem hiding this comment.
Can we add those test for MeteredTimestampedKeyValueStoreTest, too. We had the same bug in MeteredTimestampedKeyValueStore and fixed it "on the side" but never added a test...
There was a problem hiding this comment.
Will open a separate PR for that.
|
|
||
| try { | ||
| store.put("key", ValueAndTimestamp.make(42L, 60000)); | ||
| } catch (final StreamsException exception) { |
There was a problem hiding this comment.
I don't understand this ? Why do we catch StreamsException? Should the test not just fail if an exception is thrown?
There was a problem hiding this comment.
To distinguish between a test failure and a test that is terminated by an exception. If a StreamsException that caused by a ClassCastException was thrown by store.put(), then the test failed because the behaviour under test was not correct, otherwise it was terminated due to some other issues.
|
|
||
| try { | ||
| store.put("key", ValueAndTimestamp.make(42L, 60000)); | ||
| } catch (final StreamsException exception) { |
bbejeck
left a comment
bbejeck
left a comment
There was a problem hiding this comment.
Just one minor comment, otherwise LGTM
| try { | ||
| store.put("key", ValueAndTimestamp.make(42L, 60000)); | ||
| } catch (final StreamsException exception) { | ||
| if (exception.getCause() instanceof ClassCastException) { |
There was a problem hiding this comment.
nit: IMHO we should just get rid of the try/catch as if the cause is anything other than a ClassCastException this test will still pass.
There was a problem hiding this comment.
I will add an else branch and re-throw the exception. That will still distinguish between a failed and a terminated test as explained above.
There was a problem hiding this comment.
I agree with @bbejeck (compare my other comment about "why we catch StreamsException) -- if any exception is thrown, the test should fail. We don't gain anything IMHO by calling fail vs re-throwing -- it's just confusing the read the test.
There was a problem hiding this comment.
OK, I see that the tests fail in any case when an exception is thrown. However, I still think that catching StreamException and checking if ClassCastException is its cause is a good idea to document what this test is verifying. Without the try-catch it is just a couple of lines of code without any assert, which may be puzzling when read in a couple of month. Additionally, the distinction between misbehaviour of code under test (i.e. test fails with predefined message in test) and other issue (some exception was thrown) seems still relevant to me for debugging.
If you both still do not agree, I will remove the try-catch clause for the sake of getting this bugfix in.
| try { | ||
| store.put("key", ValueAndTimestamp.make(42L, 60000)); | ||
| } catch (final StreamsException exception) { | ||
| if (exception.getCause() instanceof ClassCastException) { |
|
Java 8 and Java 11 failed, test results already cleaned up. retest this please |
Fair enough, maybe we can revisit this in a follow-on PR. Merging to get the bug fix in. |
|
Merged to trunk |
…6808) Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>, Bill Bejeck <bbejeck@gmail.com>
|
cherry-picked to 2.3 |
…pache#6808) Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>, Bill Bejeck <bbejeck@gmail.com>
…pache#6808) Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>, Bill Bejeck <bbejeck@gmail.com>
4 participants
Committer Checklist (excluded from commit message)