Skip to content
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

Feature : Stop Connectors and Reset Connector Offsets #573

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

Dugong42
Copy link

@Dugong42 Dugong42 commented Oct 4, 2024

What changes did you make? (Give an overview)

This brings a few features to the "Connector" pages (list and detail), related to Connector Offsets management (#183)

Added

  • Added a dropdown option to "Stop" a connector
  • Added a dropdown option to "Reset Connector Offsets". It requires a confirmation as for the "Delete" action.
  • Added a new permission "RESET_OFFSETS" for connect
  • Displayed the "Pause" and "Resume" actions in the Connectors list page dropdown (previously only on the connector detail page)

Changes

  • The "Restart" menu of the detail page is disabled during connector state changes, instead of its individual items being disable, to give a better visual feedback of the ongoing operation.
  • The "Remove Connector" action on the connector list page is renamed as "Delete" to match the same action on the detail page and other similar actions on other resources.

Fixed

  • edit2: On the detail page, Connector actions now behave as expected, triggering a data refresh

Misc

Given I'm working behind a corporate proxy which was a pain to configure, I added a few tweaks to improve the dev experience.

  • Added some configuration to be able to build behind a corporate proxy with minimal config (for instance proxies/mirrors in .npmrc and .m2/settings.xml and system proxies)
  • Reduced maven dependency fetch time: Moved the declaration of the more specific Confluent repository after the more common Apache repo. This matters in my experience because maven tries them in order for every package.

edit1 with screenshots

The "Reset" button is disabled if the connector is not STOPPED
image

On the detail page, the "Restart" dropdown now contains the new "Stop" action
image

The other dropdown contains the Reset and Delete actions
image

Confirmation dialog
image

Is there anything you'd like reviewers to focus on?

  • Not too sure about the UX of the "Restart" dropdown. It's a bit weird to have part of the actions in it and part in the other dropdown.
  • Not sure about the current unit test policy, did not see many tests for other similar features.
  • VSCode applied some formatting changes. I hope that's not a bother to review.
  • I tested manually but was unable to run java unit tests locally, I have an 8GB RAM toaster which apparently is not enough to run WSL + the tests, as they kept getting killed by OOM errors.

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary)
  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
    • Frontend tests are passing
    • Backend tests kept crashing because of out of memory errors. My toaster with 8GB RAM was not enough to run WSL + the tests apparently.
  • Any dependent changes have been merged
    • Needs some documentation for the new permission

A picture of a cute animal (not mandatory but encouraged)

baby_kapibara

@Dugong42 Dugong42 requested review from a team as code owners October 4, 2024 15:47
@kapybro kapybro bot added status/triage Issues pending maintainers triage status/triage/manual Manual triage in progress type/feature A brand new feature status/triage/completed Automatic triage completed and removed status/triage Issues pending maintainers triage labels Oct 4, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi Dugong42! 👋

Welcome, and thank you for opening your first PR in the repo!

Please wait for triaging by our maintainers.

Please take a look at our contributing guide.

@Haarolean Haarolean linked an issue Oct 4, 2024 that may be closed by this pull request
@Haarolean Haarolean added scope/backend Related to backend changes hacktoberfest-accepted PRs accepted towards hacktoberfest goal and will be counted as approved and removed status/triage/manual Manual triage in progress labels Oct 4, 2024
@Dugong42
Copy link
Author

Dugong42 commented Oct 7, 2024

I fixed the data-refresh after connectors state changes (see description)

@Dugong42
Copy link
Author

Dugong42 commented Oct 7, 2024

There seems to be a delay between the Connect API response to the STOP operation and the connector actually being in the STOPPED state.

Since the GET to refresh the state is done immediately after this operation, it often leads to the displayed state remaining shown as RUNNING. Any subsequent refresh shows the correct state.

Copy link
Member

@Haarolean Haarolean left a comment

Choose a reason for hiding this comment

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

The backend looks good! Would be great to have some new tests as well

@Haarolean Haarolean requested a review from a team October 17, 2024 23:33
@Dugong42 Dugong42 changed the title Feature : Stop Connector and Reset Connector Offsets Feature : Stop Connectors and Reset Connector Offsets Nov 7, 2024
@Dugong42
Copy link
Author

Dugong42 commented Nov 8, 2024

I've added a couple of tests for the reset connectors feature.

I've had to update the images used in the tests to confluent 7.7.1 which is the current latest version, because this feature requires the kafka connect /connectors/{}/offsets API introduced in 7.6 (kafka 3.6) by KIP-875

The image upgrade introduced a bug in the tests because some kafka connect errors that were previously processed asynchronously are now rising up to the POST|PUT /connectors responses. See the KAFKA-14455 issue.

Specifically it now randomly produces HTTP 500 errors because Kafka Connect often falls into the "rebalancing" state when we create/delete connectors in a row.

My workaround is to optimistically ignore the connector creation error and then check for its existence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted PRs accepted towards hacktoberfest goal and will be counted as approved scope/backend Related to backend changes status/triage/completed Automatic triage completed type/feature A brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KC: Support First-class offsets in Kafka Connect
2 participants