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 2021/05/24 22:38:01 UTC
[GitHub] [kafka] ableegoldman opened a new pull request #10755: MINOR: deprecate TaskMetadata constructor, replace TaskId#parse and add tests
ableegoldman opened a new pull request #10755:
URL: https://github.com/apache/kafka/pull/10755
Quick followup to KIP-740. I also noticed the TaskId#parse method had been modified previously, and should be re-added to the public TaskId class. It also had no tests, so now it does
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639338584
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java
##########
@@ -40,6 +40,18 @@
private final Optional<Long> timeCurrentIdlingStarted;
+ /**
+ * @deprecated since 3.0, please use {@link #TaskMetadata(TaskId, Set, Map, Map, Optional) instead}
+ */
+ @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);
+ }
+
public TaskMetadata(final TaskId taskId,
Review comment:
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?)
##########
File path: docs/streams/upgrade-guide.html
##########
@@ -117,6 +117,14 @@ <h3><a id="streams_api_changes_300" href="#streams_api_changes_300">Streams API
<p>
We removed the default implementation of <code>RocksDBConfigSetter#close()</code>.
</p>
+
+ <p>
+ The public <code>topicGroupId</code> and <code>partition</code> fields on TaskId have been deprecated and replaced with getters, please migrate to using the new <code>TaskId.subtopology()</code>
+ and <code>TaskId.partition()</code> APIs instead. Also, the <code>TaskId#readFrom</code> and <code>TaskId#writeTo</code> methods have been deprecated and will be removed, as they were never intended
+ for public use to begin with. Finally, we have deprecated the <code>TaskMetadata.taskId()</code> method as well as the <code>TaskMetadata</code>constructor. These have been replaced with APIs that
Review comment:
nit: remove `to begin with` ?
##########
File path: docs/streams/upgrade-guide.html
##########
@@ -117,6 +117,14 @@ <h3><a id="streams_api_changes_300" href="#streams_api_changes_300">Streams API
<p>
We removed the default implementation of <code>RocksDBConfigSetter#close()</code>.
</p>
+
+ <p>
+ The public <code>topicGroupId</code> and <code>partition</code> fields on TaskId have been deprecated and replaced with getters, please migrate to using the new <code>TaskId.subtopology()</code>
+ and <code>TaskId.partition()</code> APIs instead. Also, the <code>TaskId#readFrom</code> and <code>TaskId#writeTo</code> methods have been deprecated and will be removed, as they were never intended
+ for public use to begin with. Finally, we have deprecated the <code>TaskMetadata.taskId()</code> method as well as the <code>TaskMetadata</code>constructor. These have been replaced with APIs that
+ better represent the task id as an actual <code>TaskId</code> object instead of a String. Please migrate to the new <code>TaskMetadata#getTaskId</code> method and the new constructor which accepts
Review comment:
`and the new constructor`
I thought that the constructor was never intended for public usage. Should we not hide the new constructor?
##########
File path: docs/streams/upgrade-guide.html
##########
@@ -117,6 +117,14 @@ <h3><a id="streams_api_changes_300" href="#streams_api_changes_300">Streams API
<p>
We removed the default implementation of <code>RocksDBConfigSetter#close()</code>.
</p>
+
+ <p>
+ The public <code>topicGroupId</code> and <code>partition</code> fields on TaskId have been deprecated and replaced with getters, please migrate to using the new <code>TaskId.subtopology()</code>
+ and <code>TaskId.partition()</code> APIs instead. Also, the <code>TaskId#readFrom</code> and <code>TaskId#writeTo</code> methods have been deprecated and will be removed, as they were never intended
+ for public use to begin with. Finally, we have deprecated the <code>TaskMetadata.taskId()</code> method as well as the <code>TaskMetadata</code>constructor. These have been replaced with APIs that
Review comment:
missing blank `<code>TaskMetadata</code>constructor`
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskId.java
##########
@@ -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) {
Review comment:
If it was removed (and release in 2.8), why add it back? Seems we don't need it?
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskId.java
##########
@@ -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('_');
Review comment:
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?
##########
File path: docs/streams/upgrade-guide.html
##########
@@ -117,6 +117,14 @@ <h3><a id="streams_api_changes_300" href="#streams_api_changes_300">Streams API
<p>
We removed the default implementation of <code>RocksDBConfigSetter#close()</code>.
</p>
+
+ <p>
+ The public <code>topicGroupId</code> and <code>partition</code> fields on TaskId have been deprecated and replaced with getters, please migrate to using the new <code>TaskId.subtopology()</code>
Review comment:
Add a hint that `topicGroupId` is renamed to `subTopologyId()` ( I know about the change and was first confused reading the sentence)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639936420
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java
##########
@@ -40,6 +40,18 @@
private final Optional<Long> timeCurrentIdlingStarted;
+ /**
+ * @deprecated since 3.0, please use {@link #TaskMetadata(TaskId, Set, Map, Map, Optional) instead}
+ */
+ @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);
+ }
+
public TaskMetadata(final TaskId taskId,
Review comment:
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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#issuecomment-848407312
Addressed your comments @mjsax
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639443906
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java
##########
@@ -40,6 +40,18 @@
private final Optional<Long> timeCurrentIdlingStarted;
+ /**
+ * @deprecated since 3.0, please use {@link #TaskMetadata(TaskId, Set, Map, Map, Optional) instead}
+ */
+ @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);
+ }
+
public TaskMetadata(final TaskId taskId,
Review comment:
> Well, we would still need a public constructor
Why can't we make it `protected`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639948650
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java
##########
@@ -40,6 +40,18 @@
private final Optional<Long> timeCurrentIdlingStarted;
+ /**
+ * @deprecated since 3.0, please use {@link #TaskMetadata(TaskId, Set, Map, Map, Optional) instead}
+ */
+ @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);
+ }
+
public TaskMetadata(final TaskId taskId,
Review comment:
Works for me. Thanks for the details.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#issuecomment-848974085
Unrelated test failures:
`DynamicBrokerConfigTest.testPasswordConfigEncoderSecretChange()`
`RaftClusterTest`
`RaftEventSimulationTest.canMakeProgressIfMajorityIsReachable`
`kafka.server.SaslApiVersionsRequestTest.[1] Type=ZK, Name=testApiVersionsRequestWithUnsupportedVersion, Security=SASL_PLAINTEXT`
Merging to trunk
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639355330
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskId.java
##########
@@ -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('_');
Review comment:
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](https://github.com/apache/kafka/pull/10615/files), 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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639356381
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskId.java
##########
@@ -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) {
Review comment:
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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639351170
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java
##########
@@ -40,6 +40,18 @@
private final Optional<Long> timeCurrentIdlingStarted;
+ /**
+ * @deprecated since 3.0, please use {@link #TaskMetadata(TaskId, Set, Map, Map, Optional) instead}
+ */
+ @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);
+ }
+
public TaskMetadata(final TaskId taskId,
Review comment:
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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman merged pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman merged pull request #10755:
URL: https://github.com/apache/kafka/pull/10755
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639358536
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java
##########
@@ -40,6 +40,18 @@
private final Optional<Long> timeCurrentIdlingStarted;
+ /**
+ * @deprecated since 3.0, please use {@link #TaskMetadata(TaskId, Set, Map, Map, Optional) instead}
+ */
+ @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);
+ }
+
public TaskMetadata(final TaskId taskId,
Review comment:
https://issues.apache.org/jira/browse/KAFKA-12849
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #10755: MINOR: deprecate TaskMetadata constructor, replace TaskId#parse and add tests
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#issuecomment-847442114
cc @mjsax
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor, replace TaskId#parse and add tests
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r638426531
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java
##########
@@ -40,6 +40,18 @@
private final Optional<Long> timeCurrentIdlingStarted;
+ /**
+ * @deprecated since 3.0, please use the constructor that accepts a TaskId object instead of a String
Review comment:
nit: It should be more clear to put a constructor link here. ex:
```
@deprecated since 3.0, please use {@link #TaskMetadata(TaskId, Set, Map, Map, Optional)} instead
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639896963
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java
##########
@@ -40,6 +40,18 @@
private final Optional<Long> timeCurrentIdlingStarted;
+ /**
+ * @deprecated since 3.0, please use {@link #TaskMetadata(TaskId, Set, Map, Map, Optional) instead}
+ */
+ @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);
+ }
+
public TaskMetadata(final TaskId taskId,
Review comment:
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](https://issues.apache.org/jira/browse/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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor, replace TaskId#parse and add tests
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r638336725
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/TaskId.java
##########
@@ -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) {
Review comment:
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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10755: MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#discussion_r639350604
##########
File path: docs/streams/upgrade-guide.html
##########
@@ -117,6 +117,14 @@ <h3><a id="streams_api_changes_300" href="#streams_api_changes_300">Streams API
<p>
We removed the default implementation of <code>RocksDBConfigSetter#close()</code>.
</p>
+
+ <p>
+ The public <code>topicGroupId</code> and <code>partition</code> fields on TaskId have been deprecated and replaced with getters, please migrate to using the new <code>TaskId.subtopology()</code>
+ and <code>TaskId.partition()</code> APIs instead. Also, the <code>TaskId#readFrom</code> and <code>TaskId#writeTo</code> methods have been deprecated and will be removed, as they were never intended
+ for public use to begin with. Finally, we have deprecated the <code>TaskMetadata.taskId()</code> method as well as the <code>TaskMetadata</code>constructor. These have been replaced with APIs that
+ better represent the task id as an actual <code>TaskId</code> object instead of a String. Please migrate to the new <code>TaskMetadata#getTaskId</code> method and the new constructor which accepts
Review comment:
True. I'll remove this note about 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #10755: MINOR: deprecate TaskMetadata constructor, replace TaskId#parse and add tests
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #10755:
URL: https://github.com/apache/kafka/pull/10755#issuecomment-847495713
All tests passed except for unrelated flaky `kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testRemovingWorker()`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org