mjsax commented on code in PR #17169:
URL: https://github.com/apache/kafka/pull/17169#discussion_r1759609910
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/RecordCollectorTest.java:
##########
@@ -1580,6 +1580,98 @@ public void
shouldThrowStreamsExceptionWhenKeySerializationFailedAndProductionEx
}
}
+ @Test
+ public void shouldNotFailIfContextIsNotAvailableOnSerializationError() {
+ try (final ErrorStringSerializer errorSerializer = new
ErrorStringSerializer()) {
+ final RecordCollector collector = new RecordCollectorImpl(
+ logContext,
+ taskId,
+ streamsProducer,
+ productionExceptionHandler,
+ streamsMetrics,
+ topology
+ );
+
+ assertThrows(
+ StreamsException.class, // should not crash with
NullPointerException
+ () -> collector.send(
+ topic,
+ "key",
+ "val",
+ null,
+ 0,
+ null,
+ errorSerializer,
+ stringSerializer,
+ sinkNodeName,
+ null // pass `null` context for testing
Review Comment:
Yes, I think it's clearer :) (otherwise, I would have done it differently).
It highlights where the `null` the test name indicates already is exactly
passed.
Of course, using `nullContext = null` and pass `nullContext` would also do
the trick. It's just that both are equally good for this case IMHO. -- There
are other cases for which I would prefer a variable, eg, `UNKNOWN = -1`, and
pass `UNKNOWN` instead of `-1` with a comment. However, for the unknown case, I
don't care how unknowns is encoded, ie, it could also be `-2` and thus using a
variable is better. For `null` vs `nullContext` there is just no read
difference, because `null` can only be `null`.
But I guess we are bikeshedding now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
