-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-5671 Followup: Remove reflections in unit test classes #3603
Conversation
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@@ -51,17 +50,17 @@ | |||
* @deprecated Use {@link org.apache.kafka.streams.StreamsBuilder StreamsBuilder} instead | |||
*/ | |||
@Deprecated | |||
public class KStreamBuilder extends TopologyBuilder { | |||
public class KStreamBuilder extends org.apache.kafka.streams.processor.TopologyBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid importing the org.apache.kafka.streams.processor.TopologyBuilder
which will introduce a WARNING during compilation; instead we will explicitly refer to the class within the class whenever we have to.
*/ | ||
@SuppressWarnings("deprecation") | ||
@Override | ||
public StateStore getStateStore(final String name) { | ||
if (currentNode() == null) { | ||
throw new TopologyBuilderException("Accessing from an unknown node"); | ||
throw new org.apache.kafka.streams.errors.TopologyBuilderException("Accessing from an unknown node"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto above.
@@ -78,4 +72,24 @@ public static Properties minimalStreamsConfig() { | |||
return results; | |||
} | |||
|
|||
// TODO: these three static functions are added because some non-TopologyBuilder unit tests need to access the internal topology builder, | |||
// which is usually a bad sign of design patterns between TopologyBuilder and StreamThread. We need to consider getting rid of them later | |||
public static InternalTopologyBuilder internalTopologyBuilder(final StreamsBuilder streamsBuilder) throws NoSuchFieldException, IllegalAccessException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and the next method. Can we wrap them with:
try {
.../
}catch(final Exception e) {
throw new RuntimeException(e);
}
We can then get rig of the throws Exception
from all the tests and from the KStreamTestDriver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to use reflection? Can we not add a test class in the same package that exposes a package private field as public? When exploring the codebase is very useful to be able to see callers and reflection kills that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ijuma that is possible. We'd need a couple of test helper classes in the appropriate packages, one for StreamBuilder
and one for InternalStreamsBuilder
. We'd also need to make internalTopologyBuilder
on StreamsBuilder
package private.
…into K5671-followup-comments
@dguy Comments addressed, could you review again? |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@guozhangwang @dguy: sorry I'm a little late with this but I was thinking instead of a static helper class we used a wrapper class instead, something like:
IMHO using this as a drop in replacement for |
retest this please. |
@bbejeck The issue is that |
@guozhangwang that's right, we'd need to add an additional helper class, let me know if you think the idea has legs, otherwise I'll just leave it as is. |
Refer to this link for build results (access rights to CI server needed): |
@bbejeck In my latest commit I'm doing similar things but in an existing class than adding additional helper classes. LMK if you find that's good enough. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @guozhangwang, LGTM.
I wonder if we should move the utility methods to get the InternalTopologyBuilder
etc onto XXUtils
classes rather than having them on specific Test classes? I don't have a strong opinion on this, so i'll leave it up to you to decide. Feel free to merge as is
@bbejeck that is what i was thinking, but we need to have at least a couple of helper classes in the appropriate packages (which is fine). I'm ok with how it is, though. TBH i'm not a big fan of either approach, but it is where we are at for the time being. |
retest this please |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@guozhangwang yeah that works for me. LGTM |
@dguy agreed on both approaches, but for now we're not relying on reflection which is the important part IMHO. |
I think it is okay to keep it in the specific test class than creating two more XXTestUtil classes. |
Merged to trunk. |
Remove rest deprecation warnings in streams:jar.
Consolidate all unit test classes' reflections to access internal topology builder from packages other than
o.a.k.streams
. We need to refactor the hierarchies of StreamTask, StreamThread and KafkaStreams to remove these hacky reflections.Minor fixes such as reference path, etc.
Minor edits on web docs for the describe function under developer-guide.