You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/11/15 20:50:33 UTC
[GitHub] [kafka] jeqo opened a new pull request, #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
jeqo opened a new pull request, #12859:
URL: https://github.com/apache/kafka/pull/12859
Changes:
- [x] Enable DEBUG log level to trigger exception
- [ ] Handle null processor supplier
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jeqo commented on a diff in pull request #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
Posted by GitBox <gi...@apache.org>.
jeqo commented on code in PR #12859:
URL: https://github.com/apache/kafka/pull/12859#discussion_r1027082893
##########
streams/src/test/resources/log4j.properties:
##########
@@ -27,7 +27,7 @@ log4j.logger.org.apache.kafka.clients=ERROR
# These are the only logs we will likely ever find anything useful in to debug Streams test failures
log4j.logger.org.apache.kafka.clients.consumer=INFO
log4j.logger.org.apache.kafka.clients.producer=INFO
-log4j.logger.org.apache.kafka.streams=INFO
+log4j.logger.org.apache.kafka.streams=DEBUG
Review Comment:
Great idea! here it is: https://github.com/apache/kafka/pull/12878
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jeqo commented on a diff in pull request #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
Posted by GitBox <gi...@apache.org>.
jeqo commented on code in PR #12859:
URL: https://github.com/apache/kafka/pull/12859#discussion_r1023922604
##########
streams/src/main/java/org/apache/kafka/streams/kstream/internals/graph/ProcessorParameters.java:
##########
@@ -130,7 +129,8 @@ public String processorName() {
@Override
public String toString() {
return "ProcessorParameters{" +
- "processor class=" + processorSupplier.get().getClass() +
+ "processor supplier class=" + (processorSupplier != null ? processorSupplier.getClass() : "null") +
+ ", fixed key processor supplier class=" + (fixedKeyProcessorSupplier != null ? fixedKeyProcessorSupplier.getClass() : "null") +
Review Comment:
Avoid calling get() as it could trigger side effects (it does on tests, see https://github.com/apache/kafka/blob/039cccca245a2314c63834e61989939738579bad/streams/src/test/java/org/apache/kafka/test/MockApiProcessorSupplier.java#L51-L54)
##########
streams/src/main/java/org/apache/kafka/streams/kstream/internals/graph/ProcessorParameters.java:
##########
@@ -114,13 +114,12 @@ KTableSource<KIn, VIn> kTableSourceSupplier() {
@SuppressWarnings("unchecked")
<KR, VR> KTableProcessorSupplier<KIn, VIn, KR, VR> kTableProcessorSupplier() {
- // This cast always works because KTableProcessorSupplier hasn't been converted yet.
Review Comment:
Now that TableProcessor suplier is using latest API, I think this is worth updating
##########
streams/src/test/resources/log4j.properties:
##########
@@ -27,7 +27,7 @@ log4j.logger.org.apache.kafka.clients=ERROR
# These are the only logs we will likely ever find anything useful in to debug Streams test failures
log4j.logger.org.apache.kafka.clients.consumer=INFO
log4j.logger.org.apache.kafka.clients.producer=INFO
-log4j.logger.org.apache.kafka.streams=INFO
+log4j.logger.org.apache.kafka.streams=DEBUG
Review Comment:
This would have triggered the error in the first place
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on PR #12859:
URL: https://github.com/apache/kafka/pull/12859#issuecomment-1321368174
Merged to trunk and backported to 3.3
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman merged pull request #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
Posted by GitBox <gi...@apache.org>.
ableegoldman merged PR #12859:
URL: https://github.com/apache/kafka/pull/12859
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jeqo commented on a diff in pull request #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
Posted by GitBox <gi...@apache.org>.
jeqo commented on code in PR #12859:
URL: https://github.com/apache/kafka/pull/12859#discussion_r1027090533
##########
streams/src/main/java/org/apache/kafka/streams/kstream/internals/graph/ProcessorParameters.java:
##########
@@ -114,13 +114,12 @@ KTableSource<KIn, VIn> kTableSourceSupplier() {
@SuppressWarnings("unchecked")
<KR, VR> KTableProcessorSupplier<KIn, VIn, KR, VR> kTableProcessorSupplier() {
- // This cast always works because KTableProcessorSupplier hasn't been converted yet.
Review Comment:
> Is that pretty much guaranteed to be a bug/result in an NPE, or are there valid uses for returning null
Looking at the uses of these methods, it could be both. There are those that expect a value and will crash with NPE (bug) (e.g. TableSourceNode), and there are others that handle null values and do something else (e.g. TableProcessorNode).
Considering the usage of these methods, we could consider removing them and handle the casting on the classes using it. I have created https://issues.apache.org/jira/browse/KAFKA-14409 to discuss this.
##########
streams/src/main/java/org/apache/kafka/streams/kstream/internals/graph/ProcessorParameters.java:
##########
@@ -114,13 +114,12 @@ KTableSource<KIn, VIn> kTableSourceSupplier() {
@SuppressWarnings("unchecked")
<KR, VR> KTableProcessorSupplier<KIn, VIn, KR, VR> kTableProcessorSupplier() {
- // This cast always works because KTableProcessorSupplier hasn't been converted yet.
Review Comment:
> Is that pretty much guaranteed to be a bug/result in an NPE, or are there valid uses for returning null
Looking at the uses of these methods, it could be both. There are those that expect a value and will crash with NPE (bug) (e.g. TableSourceNode), and there are others that handle null values and do something else (e.g. TableProcessorNode).
Considering the usage of these methods, we could consider removing them and handle the casting on the classes using it. I have created https://issues.apache.org/jira/browse/KAFKA-14409 to discuss this.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a diff in pull request #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on code in PR #12859:
URL: https://github.com/apache/kafka/pull/12859#discussion_r1027007714
##########
streams/src/test/resources/log4j.properties:
##########
@@ -27,7 +27,7 @@ log4j.logger.org.apache.kafka.clients=ERROR
# These are the only logs we will likely ever find anything useful in to debug Streams test failures
log4j.logger.org.apache.kafka.clients.consumer=INFO
log4j.logger.org.apache.kafka.clients.producer=INFO
-log4j.logger.org.apache.kafka.streams=INFO
+log4j.logger.org.apache.kafka.streams=DEBUG
Review Comment:
Hm, we might want to be careful about turning on debug logs for all tests -- it's going to explode the log file size and might slow them down, potentially by a lot. Of course debug logs are always useful for debugging (duh) and clearly expand the test coverage to some paths we aren't flexing right now, so it's a tough call
Can you actually file a separate ticket for this and extract it to its own PR?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a diff in pull request #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on code in PR #12859:
URL: https://github.com/apache/kafka/pull/12859#discussion_r1027007076
##########
streams/src/main/java/org/apache/kafka/streams/kstream/internals/graph/ProcessorParameters.java:
##########
@@ -130,7 +129,8 @@ public String processorName() {
@Override
public String toString() {
return "ProcessorParameters{" +
- "processor class=" + processorSupplier.get().getClass() +
+ "processor supplier class=" + (processorSupplier != null ? processorSupplier.getClass() : "null") +
+ ", fixed key processor supplier class=" + (fixedKeyProcessorSupplier != null ? fixedKeyProcessorSupplier.getClass() : "null") +
Review Comment:
Ah good catch
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a diff in pull request #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on code in PR #12859:
URL: https://github.com/apache/kafka/pull/12859#discussion_r1027007003
##########
streams/src/main/java/org/apache/kafka/streams/kstream/internals/graph/ProcessorParameters.java:
##########
@@ -114,13 +114,12 @@ KTableSource<KIn, VIn> kTableSourceSupplier() {
@SuppressWarnings("unchecked")
<KR, VR> KTableProcessorSupplier<KIn, VIn, KR, VR> kTableProcessorSupplier() {
- // This cast always works because KTableProcessorSupplier hasn't been converted yet.
Review Comment:
What happens if/when we call this and it returns `null`? Is that pretty much guaranteed to be a bug/result in an NPE, or are there valid uses for returning null? Sorry, wasn't keeping up with these changes so just trying to wrap my head around it
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jeqo commented on pull request #12859: KAFKA-14325: Fix NPE on Processor Parameters toString
Posted by GitBox <gi...@apache.org>.
jeqo commented on PR #12859:
URL: https://github.com/apache/kafka/pull/12859#issuecomment-1319999111
cc @ableegoldman @bbejeck @vvcephei
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org