-
Notifications
You must be signed in to change notification settings - Fork 14.9k
MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide #10755
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
Changes from all commits
01dfab9
33f9401
c04769c
bdb6786
5defad5
8387e68
1bcb66f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,11 +16,14 @@ | |
| */ | ||
| package org.apache.kafka.streams.processor; | ||
|
|
||
| import org.apache.kafka.streams.errors.TaskIdFormatException; | ||
|
|
||
| import java.io.DataInputStream; | ||
| import java.io.DataOutputStream; | ||
| import java.io.IOException; | ||
| import java.nio.ByteBuffer; | ||
| import java.util.Objects; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
|
|
@@ -80,6 +83,35 @@ public String toString() { | |
| return namedTopology != null ? namedTopology + "_" + topicGroupId + "_" + partition : topicGroupId + "_" + partition; | ||
| } | ||
|
|
||
| /** | ||
| * @throws TaskIdFormatException if the taskIdStr is not a valid {@link TaskId} | ||
| */ | ||
| public static TaskId parse(final String taskIdStr) { | ||
| final int firstIndex = taskIdStr.indexOf('_'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we plan to restrict the
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.
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 |
||
| final int secondIndex = taskIdStr.indexOf('_', firstIndex + 1); | ||
| if (firstIndex <= 0 || firstIndex + 1 >= taskIdStr.length()) { | ||
| throw new TaskIdFormatException(taskIdStr); | ||
| } | ||
|
|
||
| try { | ||
| // If only one copy of '_' exists, there is no named topology in the string | ||
| if (secondIndex < 0) { | ||
| final int topicGroupId = Integer.parseInt(taskIdStr.substring(0, firstIndex)); | ||
| final int partition = Integer.parseInt(taskIdStr.substring(firstIndex + 1)); | ||
|
|
||
| return new TaskId(topicGroupId, partition); | ||
| } else { | ||
| final String namedTopology = taskIdStr.substring(0, firstIndex); | ||
| final int topicGroupId = Integer.parseInt(taskIdStr.substring(firstIndex + 1, secondIndex)); | ||
| final int partition = Integer.parseInt(taskIdStr.substring(secondIndex + 1)); | ||
|
|
||
| return new TaskId(topicGroupId, partition, namedTopology); | ||
| } | ||
| } catch (final Exception e) { | ||
| throw new TaskIdFormatException(taskIdStr); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @throws IOException if cannot write to output stream | ||
| * @deprecated since 3.0, for internal use, will be removed | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,19 @@ public class TaskMetadata { | |
|
|
||
| private final Optional<Long> timeCurrentIdlingStarted; | ||
|
|
||
| /** | ||
| * @deprecated since 3.0, not intended for public use | ||
| */ | ||
| @Deprecated | ||
| public TaskMetadata(final String taskId, | ||
| final Set<TopicPartition> topicPartitions, | ||
| final Map<TopicPartition, Long> committedOffsets, | ||
| final Map<TopicPartition, Long> endOffsets, | ||
| final Optional<Long> timeCurrentIdlingStarted) { | ||
| this(TaskId.parse(taskId), topicPartitions, committedOffsets, endOffsets, timeCurrentIdlingStarted); | ||
| } | ||
|
|
||
| // For internal use -- not a public API | ||
| public TaskMetadata(final TaskId taskId, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why can't we make it
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 All that said I don't feel too strongly (in fact I agree with you) so I can be convinced if you do
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works for me. Thanks for the details. |
||
| final Set<TopicPartition> topicPartitions, | ||
| final Map<TopicPartition, Long> committedOffsets, | ||
|
|
||
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
toStringis part of the public TaskId API (and it should be) then thisparsemethod which does the reverse should be as well. As I discussed with some during the KIP-740debacledebate, 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