MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide by ableegoldman · Pull Request #10755 · apache/kafka

ableegoldman · 2021-05-24T22:37:49Z

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

ableegoldman · 2021-05-24T22:44:32Z

+    /**
+     * @throws TaskIdFormatException if the taskIdStr is not a valid {@link TaskId}
+     */
+    public static TaskId parse(final String taskIdStr) {


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

If it was removed (and release in 2.8), why add it back? Seems we don't need it?

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

ableegoldman · 2021-05-25T00:26:35Z

cc @mjsax

ableegoldman · 2021-05-25T03:11:13Z

All tests passed except for unrelated flaky kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testRemovingWorker()

showuon

@ableegoldman , thanks for the PR. Left a minor comment. Thanks.

mjsax · 2021-05-26T01:25:43Z

+    /**
+     * @throws TaskIdFormatException if the taskIdStr is not a valid {@link TaskId}
+     */
+    public static TaskId parse(final String taskIdStr) {


If it was removed (and release in 2.8), why add it back? Seems we don't need it?

mjsax · 2021-05-26T01:27:01Z

+        this(TaskId.parse(taskId), topicPartitions, committedOffsets, endOffsets, timeCurrentIdlingStarted);
+    }
+
    public TaskMetadata(final TaskId taskId,


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?)

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

https://issues.apache.org/jira/browse/KAFKA-12849

Well, we would still need a public constructor

Why can't we make it protected.

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

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

Works for me. Thanks for the details.

mjsax · 2021-05-26T01:28:21Z

+     * @throws TaskIdFormatException if the taskIdStr is not a valid {@link TaskId}
+     */
+    public static TaskId parse(final String taskIdStr) {
+        final int firstIndex = taskIdStr.indexOf('_');


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?

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.

ableegoldman · 2021-05-26T02:25:33Z

Addressed your comments @mjsax

mjsax

LGTM. Thanks.

ableegoldman · 2021-05-26T17:32:19Z

Unrelated test failures:
DynamicBrokerConfigTest.testPasswordConfigEncoderSecretChange()
RaftClusterTest
RaftEventSimulationTest.canMakeProgressIfMajorityIsReachable
kafka.server.SaslApiVersionsRequestTest.[1] Type=ZK, Name=testApiVersionsRequestWithUnsupportedVersion, Security=SASL_PLAINTEXT

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)

ableegoldman added 2 commits May 24, 2021 15:30

re-add and deprecate string constructor, replace parse and add tests

01dfab9

fix import order

33f9401

ableegoldman requested a review from mjsax May 24, 2021 22:37

ableegoldman added 2 commits May 24, 2021 15:41

fix toString and parse

c04769c

undo unnecessary reformatting

bdb6786

ableegoldman commented May 24, 2021

View reviewed changes

showuon reviewed May 25, 2021

View reviewed changes

Comment thread streams/src/main/java/org/apache/kafka/streams/processor/TaskMetadata.java Outdated

ableegoldman changed the title ~~MINOR: deprecate TaskMetadata constructor, replace TaskId#parse and add tests~~ MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to upgrade guide May 25, 2021

update javadocs and add notes to upgrade guide

5defad5

ableegoldman force-pushed the MINOR-KIP-740-deprecate-TaskMetadata-constructor branch from e28c3e6 to 5defad5 Compare May 25, 2021 04:59

mjsax reviewed May 26, 2021

View reviewed changes

review feedback

8387e68

formatting

1bcb66f

mjsax approved these changes May 26, 2021

View reviewed changes

ableegoldman merged commit bbe170a into apache:trunk May 26, 2021

Conversation

ableegoldman commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ableegoldman commented May 25, 2021

Uh oh!

ableegoldman commented May 25, 2021

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ableegoldman commented May 26, 2021

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

ableegoldman commented May 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ableegoldman commented May 24, 2021 •

edited

Loading