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

KAFKA-18209 Cleanup __transaction_state config logic #18201

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from

Conversation

wernerdv
Copy link
Contributor

@wernerdv wernerdv commented Dec 15, 2024

Since the EnforcedCompression and EnforcedRequiredAcks fields are used only in the TransactionStateManager class, I moved them to a companion object.

Committer Checklist (excluded from commit message)

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

@github-actions github-actions bot added triage PRs from the community core Kafka Broker small Small PRs labels Dec 15, 2024
Copy link

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

@wernerdv
Copy link
Contributor Author

@mimaison Can you tell me if there's anything I can do to improve my PR?

Copy link

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

Copy link
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I left a suggestion.

// 2. compression = none
// 3. unclean leader election = disabled
// 4. required acks = -1 when writing
val EnforcedCompression: Compression = Compression.NONE
Copy link
Member

Choose a reason for hiding this comment

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

We also want to use these fields in transactionTopicConfigs()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I corrected it.
Is that what you wanted?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is the best place to store these values. Have you considered TransactionCoordinator? I'm not saying it should go there, just asking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since TransactionStateManager#transactionTopicConfigs is used only in TransactionCoordinator, it makes sense to store these values in TransactionCoordinator.

Copy link
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

When I opened the ticket I was hoping we could do a bit more cleanup, but maybe there isn't much to do. The point of the Jira was to investigate and see what could be done.

I left a couple of comments on the changes you propose.

// 2. compression = none
// 3. unclean leader election = disabled
// 4. required acks = -1 when writing
val EnforcedCompression: Compression = Compression.NONE
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is the best place to store these values. Have you considered TransactionCoordinator? I'm not saying it should go there, just asking.

@github-actions github-actions bot removed triage PRs from the community needs-attention labels Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Kafka Broker small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants