KAFKA-3597: Query ConsoleConsumer and VerifiableProducer if they shutdown cleanly#1278
KAFKA-3597: Query ConsoleConsumer and VerifiableProducer if they shutdown cleanly#1278apovzner wants to merge 5 commits into
Conversation
…ey shutdown cleanly
|
|
||
| shutdownLatch.await() | ||
|
|
||
| System.out.println("shutdown_complete") |
There was a problem hiding this comment.
Is there a risk of breaking tools which depend on console consumer to read messages? You'd need to filter this message if you're doing any processing on the results. Would it be better to add an option to console consumer to explicitly enable this? For example, "--enable-lifecycle-logging."
There was a problem hiding this comment.
Or maybe just log at trace level perhaps? Looks like console_consumer.py is logging at TRACE, so we should pick this up?
There was a problem hiding this comment.
Yeah, that might work, but it seems a little weird for a test's correctness to depend on the log level. Do we do that elsewhere?
There was a problem hiding this comment.
Agree. I like the --enable-lifecycle-logging idea. I will do that.
|
@hachikuji and @granders Thanks a lot for your comments. I addressed all of them. Could you take one more look? |
| .withRequiredArg | ||
| .describedAs("deserializer for values") | ||
| .ofType(classOf[String]) | ||
| val enableLifecycleLoggingOpt = parser.accepts("enable-lifecycle-logging", "Log lifecycle events of the consumer.") |
There was a problem hiding this comment.
My main question about this approach is that I'm not sure which lifecycle events are best to "log". If we're providing this flag, it seems surprising to only log a clean shutdown.
And, does "log" mean print?
There was a problem hiding this comment.
I think it's better to extend to more events later when we use them.. VerifiableConsumer has more of these events. But it also seems ok to add a flag for future additions of events when needed.
I think you can use both 'print' or 'log' as a more general term, no? logging to standard output
There was a problem hiding this comment.
Maybe for now, we could just log startup and shutdown?
| self.from_beginning = from_beginning | ||
| self.message_validator = message_validator | ||
| self.messages_consumed = {idx: [] for idx in range(1, num_nodes + 1)} | ||
| self.node_indexes_clean_shutdown = set() |
There was a problem hiding this comment.
Nitpick: the name is a little awkward. Maybe it would be clear enough even if you leave out "indexes" (e.g. clean_shutdown_nodes)?
There was a problem hiding this comment.
@hachikuji Actually I asked for a name which made it clearer that this object contains numerical indices, and not actual node objects... :)
There was a problem hiding this comment.
Haha, fair enough. Another option would be to actually use the node objects in the set. Anyway, just a nitpick.
There was a problem hiding this comment.
Ok lets just do nodes in the set: clean_shutdown_nodes
|
@granders and @hachikuji Ready for another look. |
|
LGTM |
1 similar comment
|
LGTM |
…down cleanly Even if a test calls stop() on console_consumer or verifiable_producer, it is still possible that producer/consumer will not shutdown cleanly, and will be killed forcefully after a timeout. It will be useful for some tests to know whether a clean shutdown happened or not. This PR adds methods to console_consumer and verifiable_producer to query whether clean shutdown happened or not. hachikuji and/or granders Please review. Author: Anna Povzner <anna@confluent.io> Reviewers: Jason Gustafson, Geoff Anderson, Gwen Shapira Closes apache#1278 from apovzner/kafka-3597
* Impl for writeState in DefaultSharePersister, ShareCoordinatorService and ShareCoordinatorShard * Setup in KafkaApis to intercept the RPC and return the result.
4 participants
Even if a test calls stop() on console_consumer or verifiable_producer, it is still possible that producer/consumer will not shutdown cleanly, and will be killed forcefully after a timeout. It will be useful for some tests to know whether a clean shutdown happened or not. This PR adds methods to console_consumer and verifiable_producer to query whether clean shutdown happened or not.
@hachikuji and/or @granders Please review.