cadonna commented on code in PR #18835:
URL: https://github.com/apache/kafka/pull/18835#discussion_r1949001565
##########
streams/src/test/java/org/apache/kafka/streams/tests/SmokeTestDriver.java:
##########
@@ -616,14 +618,17 @@ private static boolean verifyTAgg(final PrintStream
resultStream,
final Map<String, Set<Integer>> allData,
final Map<String,
LinkedList<ConsumerRecord<String, Number>>> taggEvents,
final boolean printResults) {
+ resultStream.println("verifying tagg");
Review Comment:
What is `tagg`? Something like `verifying topic tagg` would be clearer.
##########
streams/src/test/java/org/apache/kafka/streams/tests/SmokeTestDriver.java:
##########
@@ -521,13 +521,16 @@ private static boolean verify(final PrintStream
resultStream,
final Map<String, Map<String,
LinkedList<ConsumerRecord<String, Number>>>> events,
final Function<String, Number>
keyToExpectation,
final boolean printResults) {
+ resultStream.printf("verifying topic '%s'%n", topic);
Review Comment:
Something seems odd here. You have two format specifiers, `%s` and `%n`, but
only one argument `topic`.
##########
tests/kafkatest/tests/streams/streams_smoke_test.py:
##########
@@ -109,5 +109,7 @@ def test_streams(self, processing_guarantee, crash,
metadata_quorum):
if crash and processing_guarantee == 'at_least_once':
self.driver.node.account.ssh("grep -E
'SUCCESS|PROCESSED-MORE-THAN-GENERATED' %s" % self.driver.STDOUT_FILE,
allow_fail=False)
+ # fail if we find "missing result data" output in the stdout file;
while we can tolerate duplication, we cannot tolerate data loss
+ self.driver.node.account.ssh("[ ! `grep 'missing result data'" %
self.driver.STDOUT_FILE % "` ]", allow_fail=False)
else:
self.driver.node.account.ssh("grep SUCCESS %s" %
self.driver.STDOUT_FILE, allow_fail=False)
Review Comment:
Couldn't we test for `FAILURE` here instead?
##########
streams/src/test/java/org/apache/kafka/streams/tests/SmokeTestDriver.java:
##########
@@ -521,13 +521,16 @@ private static boolean verify(final PrintStream
resultStream,
final Map<String, Map<String,
LinkedList<ConsumerRecord<String, Number>>>> events,
final Function<String, Number>
keyToExpectation,
final boolean printResults) {
+ resultStream.printf("verifying topic '%s'%n", topic);
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
observedInputEvents = events.get("data");
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
outputEvents = events.getOrDefault(topic, emptyMap());
if (outputEvents.isEmpty()) {
- resultStream.println(topic + " is empty");
+ resultStream.println("missing result data; topic '" + topic + "'
is empty, expected " + inputData.size() + " keys");
return false;
} else {
- resultStream.printf("verifying %s with %d keys%n", topic,
outputEvents.size());
+ if (outputEvents.size() < inputData.size()) {
+ resultStream.println("missing result data; got " +
inputData.size() + " keys, expected: " + outputEvents.size() + " keys");
Review Comment:
Also here, a `fail: ` at the beginning of the message would it make clearer.
##########
streams/src/test/java/org/apache/kafka/streams/tests/SmokeTestDriver.java:
##########
@@ -521,13 +521,16 @@ private static boolean verify(final PrintStream
resultStream,
final Map<String, Map<String,
LinkedList<ConsumerRecord<String, Number>>>> events,
final Function<String, Number>
keyToExpectation,
final boolean printResults) {
+ resultStream.printf("verifying topic '%s'%n", topic);
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
observedInputEvents = events.get("data");
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
outputEvents = events.getOrDefault(topic, emptyMap());
if (outputEvents.isEmpty()) {
- resultStream.println(topic + " is empty");
+ resultStream.println("missing result data; topic '" + topic + "'
is empty, expected " + inputData.size() + " keys");
return false;
} else {
- resultStream.printf("verifying %s with %d keys%n", topic,
outputEvents.size());
+ if (outputEvents.size() < inputData.size()) {
+ resultStream.println("missing result data; got " +
inputData.size() + " keys, expected: " + outputEvents.size() + " keys");
Review Comment:
Did you forget the `return false;` here?
##########
streams/src/test/java/org/apache/kafka/streams/tests/SmokeTestDriver.java:
##########
@@ -521,13 +521,16 @@ private static boolean verify(final PrintStream
resultStream,
final Map<String, Map<String,
LinkedList<ConsumerRecord<String, Number>>>> events,
final Function<String, Number>
keyToExpectation,
final boolean printResults) {
+ resultStream.printf("verifying topic '%s'%n", topic);
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
observedInputEvents = events.get("data");
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
outputEvents = events.getOrDefault(topic, emptyMap());
if (outputEvents.isEmpty()) {
- resultStream.println(topic + " is empty");
+ resultStream.println("missing result data; topic '" + topic + "'
is empty, expected " + inputData.size() + " keys");
Review Comment:
Great! The old output confused me because it was not immediately clear that
it was a failure.
##########
streams/src/test/java/org/apache/kafka/streams/tests/SmokeTestDriver.java:
##########
@@ -521,13 +521,16 @@ private static boolean verify(final PrintStream
resultStream,
final Map<String, Map<String,
LinkedList<ConsumerRecord<String, Number>>>> events,
final Function<String, Number>
keyToExpectation,
final boolean printResults) {
+ resultStream.printf("verifying topic '%s'%n", topic);
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
observedInputEvents = events.get("data");
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
outputEvents = events.getOrDefault(topic, emptyMap());
if (outputEvents.isEmpty()) {
- resultStream.println(topic + " is empty");
+ resultStream.println("missing result data; topic '" + topic + "'
is empty, expected " + inputData.size() + " keys");
return false;
} else {
- resultStream.printf("verifying %s with %d keys%n", topic,
outputEvents.size());
+ if (outputEvents.size() < inputData.size()) {
+ resultStream.println("missing result data; got " +
inputData.size() + " keys, expected: " + outputEvents.size() + " keys");
+ }
if (outputEvents.size() != inputData.size()) {
Review Comment:
Would it make sense to merge the previous check with this check?
If the previous check is `false` also this is `false`. The inverse is not
`true`, though.
##########
streams/src/test/java/org/apache/kafka/streams/tests/SmokeTestDriver.java:
##########
@@ -521,13 +521,16 @@ private static boolean verify(final PrintStream
resultStream,
final Map<String, Map<String,
LinkedList<ConsumerRecord<String, Number>>>> events,
final Function<String, Number>
keyToExpectation,
final boolean printResults) {
+ resultStream.printf("verifying topic '%s'%n", topic);
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
observedInputEvents = events.get("data");
final Map<String, LinkedList<ConsumerRecord<String, Number>>>
outputEvents = events.getOrDefault(topic, emptyMap());
if (outputEvents.isEmpty()) {
- resultStream.println(topic + " is empty");
+ resultStream.println("missing result data; topic '" + topic + "'
is empty, expected " + inputData.size() + " keys");
Review Comment:
maybe a `fail: ` at the beginning of the message would make it even clearer.
--
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]
