Skip to content

#2 Add 'get offsets' command#338

Merged
gunnarmorling merged 1 commit into
kcctl:mainfrom
C0urante:gh-2
Jul 27, 2023
Merged

#2 Add 'get offsets' command#338
gunnarmorling merged 1 commit into
kcctl:mainfrom
C0urante:gh-2

Conversation

@C0urante

@C0urante C0urante commented Jul 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Addresses #2

Usage:

Usage: kcctl get offsets [NAME...]
Displays committed connector offsets
      [NAME...]   Name of the connector (e.g. 'my-connector')

Uses the GET /connectors/{name}/offsets endpoint introduced in KIP-875 to retrieve committed offsets for a connector.

Since this feature requires Kafka Connect 3.5 or later, the integration test is customized to use a non-stable version of Debezium (2.4.0 alpha). We can and should update the version used in that test once a final release takes place for 2.4.0.

@C0urante

C0urante commented Jul 26, 2023

Copy link
Copy Markdown
Contributor Author

@gunnarmorling if you have a moment, mind taking a look at this one? Thanks!

gunnarmorling
gunnarmorling reviewed Jul 26, 2023
View reviewed changes

@gunnarmorling gunnarmorling left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woot, woot, LOVING it 😍! This has been on the roadmap for sooo long (I mean, it's issue #2 after all :). Thanks a ton for picking this up.

Looks great to me overall, a few minor comments inline. Besides those, taking a step back, instead of making this a new sub-command of get, did you consider making this part of describe connector, e.g. via a new switch akin to --tasks-config? I feel this would be somewhat more intuitive/user-friendly, as I wouldn't consider offsets to be their own entity that much as just a property of the connector entity. Curious what you think.

Once we've made a decision on that one,

  • The completion file should be updated as part of this PR, too
  • The help output in the README should be updated if it changes at all (if we stick with get offsets, then the help output of get should mention that)

Thanks again!

Comment thread src/main/java/org/kcctl/command/GetOffsetsCommand.java
Comment thread src/test/java/org/kcctl/command/GetOffsetsCommandTest.java
Comment thread src/test/java/org/kcctl/command/GetOffsetsCommandTest.java Outdated

Instant start = Instant.now();

while (Duration.between(start, Instant.now()).toSeconds() < 30) {

@gunnarmorling gunnarmorling Jul 26, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good use case for Awaitility. Let's add it as a dependency and use it here if you like the idea. It would make that test a bit nicer to read, but it's also not a strict requirement from my PoV.

@C0urante C0urante Jul 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it! Done. The one small issue I found is that (with the five minutes of docs perusal I did) there doesn't seem to be an easy way to emit a custom error message if an awaited condition never evaluates to true, but it still seems worth the tradeoff in improved readability, especially since (fingers crossed) it's unlikely these tests will start spontaneously failing.

@C0urante

C0urante commented Jul 26, 2023

Copy link
Copy Markdown
Contributor Author

Thanks Gunnar! Planning to push a new commit tomorrow to address the suggestions.

RE folding this into describe connector--this is really interesting, and I see the argument for intuitiveness. One question that comes to mind is if that approach will remain as intuitive when we add support for altering/resetting connector offsets. I think for altering offsets we could possibly fold it into patch connector via something like --offsets (though this feels like it might be giving that command a little too much to do), but for resetting them entirely, I'm not sure what we could do... delete connector --offsets doesn't seem great, patch connector --reset-offsets seems better, but then we have describe connector --offsets, patch connector --offsets, and patch connector --reset-offsets vs. get offsets, patch offsets, and delete offsets, which doesn't seem inherently superior IMO.

Thoughts on which syntax to choose with the future offsets-related commands (that I *promise* to implement after Kafka 3.6.0 is released) in mind?

@gunnarmorling

gunnarmorling commented Jul 27, 2023

Copy link
Copy Markdown
Collaborator

Good point about those future offset related APIs. Looks like making offsets their own first-level entity as you propose is more practical indeed.

We still may consider later on whether to add a flag --offsets to describe connector, so as to get everything connector-related at once (and I think examining offsets will be a more common operation than changing or deleting them). It'd be a bit redundant, though.

I'd say let's go with your proposal and take it from there.

@C0urante C0urante mentioned this pull request Jul 27, 2023
Closed
@C0urante

C0urante commented Jul 27, 2023

Copy link
Copy Markdown
Contributor Author

Thanks Gunnar! I've updated the README and completion script, and addressed your other inline code suggestions. This is ready for another pass when you have time.

@C0urante

C0urante commented Jul 27, 2023

Copy link
Copy Markdown
Contributor Author

Hmm... the RestartConnectorCommandTest::should_restart_only_once_with_regexp test case failed on the latest CI build. The command invocation completed with a non-zero exit status. I wonder if this is because we don't wait for connectors to be up and running before restarting them? If we issue a restart request too soon after creating one or more connectors, it may get rejected by the Connect cluster if a rebalance is about to happen.

Kicking off another CI run. If this is only slightly flaky, I'd like to file a follow-up issue to investigate further; if it occurs frequently enough to be annoying, then we should probably investigate further before merging this PR to make sure that it's not caused by these changes.

@C0urante

C0urante commented Jul 27, 2023

Copy link
Copy Markdown
Contributor Author

Latest run succeeded. Kicking off one more just to be safe.

@gunnarmorling

gunnarmorling commented Jul 27, 2023

Copy link
Copy Markdown
Collaborator

Yeah, we definitely have some issues with flaky tests. Last one just passed, will merge.

If this is only slightly flaky, I'd like to file a follow-up issue to investigate further

Yes, if you could log one, that'd be great. Not sure when this started, also saw this when merging a few Dependabot PRs yesterday. So it definitely shouldn't be related by this PR.

Thanks again, really cool to see this finally becoming reality in kcctl!

@gunnarmorling gunnarmorling merged commit 5984316 into kcctl:main Jul 27, 2023
@C0urante C0urante deleted the gh-2 branch July 27, 2023 21:06
This was referenced Jul 27, 2023
Closed
Closed
@C0urante

C0urante commented Jul 27, 2023

Copy link
Copy Markdown
Contributor Author

Thanks Gunnar! Logged #353 regarding the flaky integration tests.

@C0urante C0urante mentioned this pull request Jul 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gunnarmorling gunnarmorling gunnarmorling left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@C0urante @gunnarmorling