-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide #10755
MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide #10755
Conversation
/** | ||
* @throws TaskIdFormatException if the taskIdStr is not a valid {@link TaskId} | ||
*/ | ||
public static TaskId parse(final String taskIdStr) { |
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.
I noticed this has been (re)moved since 2.8 so I put it back with updates to handle named topologies, plus tests which it did not seem to have
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.
If it was removed (and release in 2.8), why add it back? Seems we don't need it?
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.
Well that would be a breaking change by removing a non-deprecated API, no? And in this case I actually believe we should not deprecate it -- if toString
is part of the public TaskId API (and it should be) then this parse
method which does the reverse should be as well. As I discussed with some during the KIP-740 debacle debate, part of the public contract of TaskId is in its string representation since that is what ends up in logs, metrics, etc. So imo it does make sense to provide a String-to-TaskId API
cc @mjsax |
All tests passed except for unrelated flaky |
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.
@ableegoldman , thanks for the PR. Left a minor comment. Thanks.
streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java
Outdated
Show resolved
Hide resolved
e28c3e6
to
5defad5
Compare
/** | ||
* @throws TaskIdFormatException if the taskIdStr is not a valid {@link TaskId} | ||
*/ | ||
public static TaskId parse(final String taskIdStr) { |
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.
If it was removed (and release in 2.8), why add it back? Seems we don't need it?
final Optional<Long> timeCurrentIdlingStarted) { | ||
this(TaskId.parse(taskId), topicPartitions, committedOffsets, endOffsets, timeCurrentIdlingStarted); | ||
} | ||
|
||
public TaskMetadata(final TaskId taskId, |
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.
Should we make this protected and add a internal "impl" class? (And also mark as non-public so we can eventually move to an interface?)
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.
Well, we would still need a public constructor no matter what even with an internal impl class. I'll add a comment to clarify that it's not intended for public use and maybe file a followup ticket to migrate this to an interface in case someone wants to get into 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.
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.
Well, we would still need a public constructor
Why can't we make it protected
.
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.
I meant it seems like we should need it as long as TaskMetadata continues to be a public class, ie if we take away the constructor then we've essentially all but made it into an interface already -- which was not what was agreed on in KIP-740, and I don't want to drag this out further if there is any pushback...
Imo it's sufficient to just leave the constructor as public
but not present it as a public API, and leave the rest for the KIP for KAFKA-12849. It's a simple KIP so I'm optimistic someone new to kafka may want to pick it up 🙂
All that said I don't feel too strongly (in fact I agree with you) so I can be convinced if you do
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.
In fact we already got a bite on KAFKA-12849 -- if it's all the same to you then I'd prefer to just let this be handled by that KIP. Assuming we get it in by 3.0 then there won't ever be a new public constructor published for TaskMetadata
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.
Works for me. Thanks for the details.
* @throws TaskIdFormatException if the taskIdStr is not a valid {@link TaskId} | ||
*/ | ||
public static TaskId parse(final String taskIdStr) { | ||
final int firstIndex = taskIdStr.indexOf('_'); |
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.
I guess I missed the "name topology" change. What is a topology name? How to set it? And do we ensure that we don't allow _
in its name?
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.
Yes, we plan to restrict the _
character. If we want to loosen that up later we can just parse this from the back, but I think it's reasonable to just disallow _
completely.
What is a topology name?
Great question. Not necessarily a short answer but I can try -- basically an independent and isolated piece of a topology that can be added/removed/etc at will, even on a running app.
How to set it?
The skeleton API was merged in #10615, it has/is evolving a bit since then but the basic idea holds -- each NamedTopology is built up with a special builder called the NamedTopologyStreamsBuilder. And a dedicated KafkaStreams wrapper is the entry point for starting up an app using NamedTopologies. All currently under the internals
package while it's under the experimental phase so it should not be possible for a user to end up with anything NamedTopology through public APIs.
Addressed your comments @mjsax |
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.
LGTM. Thanks.
Unrelated test failures: Merging to trunk |
…nups * apache-github/trunk: MINOR: Adjust parameter ordering of `waitForCondition` and `retryOnExceptionWithTimeout` (apache#10759) KAFKA-12796: Removal of deprecated classes under streams-scala (apache#10710) KAFKA-12819: Add assert messages to MirrorMaker tests plus other quality of life improvements (apache#10762) Update implementation.html (apache#10771) MINOR: Log constructor: Flip logical NOT for readability (apache#10756) MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide (apache#10755) MINOR: Log more information when producer snapshot is written (apache#10757) KAFKA-12260: Avoid hitting NPE for partitionsFor (apache#10017) MINOR: add window verification to sliding-window co-group test (apache#10745) KAFKA-12800: Configure generator to fail on trailing JSON tokens (apache#10717)
Quick followup to KIP-740 to actually deprecate this constructor, and update the upgrade guide with what we changed in KIP-740. I also noticed the TaskId#parse method had been modified previously, and should be re-added to the public TaskId class. It had no tests, so now it does