KAFKA-8231: Expansion of ConnectClusterState interface by C0urante · Pull Request #6584 · apache/kafka

C0urante · 2019-04-15T18:23:12Z

The changes here add new methods to the ConnectClusterState interface so that Connect REST extensions can be more aware of the current state of the Connect cluster they are added to. The new methods allow extensions to query for connector and task configurations, as well as the ID of the Kafka cluster targeted by the Connect cluster.

All new methods have new unit tests added for their implementations in the ConnectClusterStateImpl class.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

C0urante · 2019-04-30T20:25:26Z

Rebased on current trunk. There are unit test failures, but I believe most are due to KAFKA-8304. Will address those after that issue has been resolved.

C0urante · 2019-05-03T00:18:08Z

Blocked by KAFKA-8304, a fix for which is currently underway at #6651

…tClusterDetails interface and implementation

C0urante · 2019-05-07T22:52:58Z

Unblocked as a fix for KAFKA-8304 has been merged. Now ready for review.

C0urante · 2019-05-09T02:23:13Z

@rhauch now that the KIP has been accepted, would you mind taking a look and reviewing these changes?

kkonstantine

Thanks @C0urante !
Looks pretty good overall. Left a few minor comments.

kkonstantine · 2019-05-09T18:23:24Z

+
+import org.apache.kafka.connect.health.ConnectClusterDetails;
+
+public class ConnectClusterDetailsImpl implements ConnectClusterDetails {


Still a javadoc would be nice, stating that this is a basic/simple implementation of ConnectClusterDetails meant to be used for ... etc

There isn't a matching javadoc on the ConnectClusterStateImpl class and I'm not sure it'd be too useful to add one here. If we mention that it's explicitly for use in REST extensions, that information may become outdated if/when the class is used elsewhere. There's pretty sufficient detail on the behavior of the class provided in the javadocs for the interface it implements. I don't think there's anything non-obvious that needs to be documented about this class.

Not a blocker for me, but I think any javadoc is better than no javadoc for a class or an interface.
At the class level, the javadoc is not inherited by the base class (as when @Override is used in methods). So the presence of javadoc in the interface but not here does not save us the extra hoop. In any case, I mentioned since this is a small PR and I think it's a good practice (that at least I try to follow as much as possible). Not a blocker for this PR.

kkonstantine · 2019-05-09T18:28:19Z

+        final Map<String, String> expectedConfig = Collections.singletonMap("key", "value");
+        Capture<Callback<Map<String, String>>> callback = EasyMock.newCapture();
+        herder.connectorConfig(EasyMock.eq(connName), EasyMock.capture(callback));
+        EasyMock.expectLastCall().andAnswer(new IAnswer<Void>() {


should we start writing those as lambdas for the new code (the code that is not meant to be backported prior to 2.0?)

I wouldn't mind altering these to be lambdas but it'd be better if we could backport that change to 2.0 to avoid any merge conflicts down the road if a bug fix is discovered that needs to be backported that far as well. Since this PR is targeted at 2.3 I think we should hold off on making those changes here.

I was referring only to the newly introduced methods and their tests. Not the whole class. Not a big issue.

I think introducing mixed style here would just cause minor confusion--why use lambdas for some test cases but not for others? It wouldn't be obvious to someone without looking through the git history of the class and maybe even this PR discussion.

wicknicks

LGTM. Thanks, @C0urante.

C0urante · 2019-05-09T20:14:02Z

Thank you for the review, @wicknicks and @kkonstantine! Konstantine--I've either responded to or made changes based off each of your comments; would appreciate a second round when you have time.

kkonstantine

Thanks @C0urante!
The open discussions are not blockers.
LGTM

rhauch

Nice work, @C0urante. The implementations are clear and straightforward, and this is fine without the few optional suggestions. Thanks for putting this together.

…es-14-May * AK_REPO/trunk: (24 commits) KAFKA-7321: Add a Maximum Log Compaction Lag (KIP-354) (apache#6009) KAFKA-8335; Clean empty batches when sequence numbers are reused (apache#6715) KAFKA-6455: Session Aggregation should use window-end-time as record timestamp (apache#6645) KAFKA-6521: Use timestamped stores for KTables (apache#6667) [MINOR] Consolidate in-memory/rocksdb unit tests for window & session store (apache#6677) MINOR: Include StickyAssignor in system tests (apache#5223) KAFKA-7633: Allow Kafka Connect to access internal topics without cluster ACLs (apache#5918) MINOR: Align KTableAgg and KTableReduce (apache#6712) MINOR: Fix code section formatting in TROGDOR.md (apache#6720) MINOR: Remove unnecessary OptionParser#accepts method call from PreferredReplicaLeaderElectionCommand (apache#6710) KAFKA-8352 : Fix Connect System test failure 404 Not Found (apache#6713) KAFKA-8348: Fix KafkaStreams JavaDocs (apache#6707) MINOR: Add missing option for running vagrant-up.sh with AWS to vagrant/README.md KAFKA-8344; Fix vagrant-up.sh to work with AWS properly MINOR: docs typo in '--zookeeper myhost:2181--execute' MINOR: Remove header and key/value converter config value logging (apache#6660) KAFKA-8231: Expansion of ConnectClusterState interface (apache#6584) KAFKA-8324: Add close() method to RocksDBConfigSetter (apache#6697) KAFKA-6789; Handle retriable group errors in AdminClient API (apache#5578) KAFKA-8332: Refactor ImplicitLinkedHashSet to avoid losing ordering when converting to Scala ...

Expand ConnectClusterState interface and implementation with methods that provide the immutable cluster details and the connector configuration. This includes unit tests for the new methods. Author: Chris Egerton <cegerton@oberlin.edu> Reviews: Arjun Satish <arjun@confluent.io>, Konstantine Karantasis <konstantine@confluent.io>, Randall Hauch <rhauch@gmail.com>

C0urante force-pushed the kafka-8231 branch from 8a94b72 to 4611997 Compare April 30, 2019 20:24

C0urante added 6 commits May 7, 2019 15:29

KAFKA-8231: Expand ConnectClusterState interface

f5fb018

KAFKA-8231: Add unit tests for new ConnectClusterStateImpl methods

80676c9

KAFKA-8231: Remove ConnectClusterState.taskConfigs method, add Connec…

93adb44

…tClusterDetails interface and implementation

KAFKA-8231: Minor stylistic cleanup

5d41d3e

KAFKA-8231: Minor Javadoc tweak

a2b6b8a

KAFKA-8231: Fix failing unit test

f1d7444

C0urante force-pushed the kafka-8231 branch from bf1cf1a to f1d7444 Compare May 7, 2019 22:52

kkonstantine reviewed May 9, 2019

View reviewed changes

wicknicks approved these changes May 9, 2019

View reviewed changes

KAFKA-8231: Address review comments

3f5e0f4

kkonstantine approved these changes May 9, 2019

View reviewed changes

rhauch approved these changes May 10, 2019

View reviewed changes

rhauch merged commit 36a5aba into apache:trunk May 10, 2019

kkonstantine added the connect label Oct 16, 2020

C0urante deleted the kafka-8231 branch February 27, 2022 03:56


		import org.apache.kafka.connect.health.ConnectClusterDetails;

		public class ConnectClusterDetailsImpl implements ConnectClusterDetails {

Conversation

C0urante commented Apr 15, 2019

Committer Checklist (excluded from commit message)

Uh oh!

C0urante commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

C0urante commented May 3, 2019

Uh oh!

C0urante commented May 7, 2019

Uh oh!

C0urante commented May 9, 2019

Uh oh!

kkonstantine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kkonstantine May 9, 2019

Choose a reason for hiding this comment

Uh oh!

C0urante May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkonstantine May 9, 2019

Choose a reason for hiding this comment

Uh oh!

kkonstantine May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0urante May 9, 2019

Choose a reason for hiding this comment

Uh oh!

kkonstantine May 9, 2019

Choose a reason for hiding this comment

Uh oh!

C0urante May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wicknicks left a comment

Choose a reason for hiding this comment

Uh oh!

C0urante commented May 9, 2019

Uh oh!

kkonstantine left a comment

Choose a reason for hiding this comment

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

C0urante commented Apr 30, 2019 •

edited

Loading

C0urante May 9, 2019 •

edited

Loading

kkonstantine May 9, 2019 •

edited

Loading

C0urante May 10, 2019 •

edited

Loading