Skip to content

KAFKA-10852: AlterIsr should not be throttled#9747

Merged
ijuma merged 4 commits into
apache:trunkfrom
ijuma:alter-isr-cluster-action
Dec 15, 2020
Merged

KAFKA-10852: AlterIsr should not be throttled#9747
ijuma merged 4 commits into
apache:trunkfrom
ijuma:alter-isr-cluster-action

Conversation

@ijuma

@ijuma ijuma commented Dec 14, 2020

Copy link
Copy Markdown
Member

Set it as a cluster action and update the handler in KafkaApis. We keep the throttleTimeMs field
since we intend to enable throttling in the future (especially relevant when we switch to the
built-in quorum mode).

Committer Checklist (excluded from commit message)

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

This ensures it won't get throttled.
@ijuma ijuma requested review from hachikuji and mumrah December 14, 2020 15:01
List<ApiKeys> authenticationKeys = Arrays.asList(ApiKeys.SASL_HANDSHAKE, ApiKeys.SASL_AUTHENTICATE);
Set<ApiKeys> authenticationKeys = EnumSet.of(ApiKeys.SASL_HANDSHAKE, ApiKeys.SASL_AUTHENTICATE);
// Newer protocol apis include throttle time ms even for cluster actions
Set<ApiKeys> clusterActionsWithThrottleTimeMs = EnumSet.of(ApiKeys.ALTER_ISR);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We have to decide whether we want to do this or remove the throttle time ms from AlterIsr before 2.7.0.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We decided this is not a blocker.

@mumrah mumrah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ijuma! LGTM

@ijuma ijuma changed the title KAFKA-10852: AlterIsr should be a cluster action KAFKA-10852: AlterIsr should not be throttled Dec 14, 2020
@ijuma

ijuma commented Dec 14, 2020

Copy link
Copy Markdown
Member Author

@mumrah A couple of the test failures indicated that we had to change some code in KafkaApis too. Updated.

@mumrah

mumrah commented Dec 14, 2020

Copy link
Copy Markdown
Member

@ijuma, thanks! So, we'll keep the throttle field in the RPC, but we won't actually set the throttle. Seems reasonable 👍

@ijuma

ijuma commented Dec 15, 2020

Copy link
Copy Markdown
Member Author

Unrelated flaky test failures:

Build / JDK 11 / kafka.server.ScramServerStartupTest.testAuthentications
Build / JDK 15 / org.apache.kafka.streams.integration.StandbyTaskEOSIntegrationTest.shouldWipeOutStandbyStateDirectoryIfCheckpointIsMissing[exactly_once]

@ijuma ijuma merged commit 5e5daf4 into apache:trunk Dec 15, 2020
@ijuma ijuma deleted the alter-isr-cluster-action branch December 15, 2020 06:28
ijuma added a commit that referenced this pull request Dec 15, 2020
Set it as a cluster action and update the handler in KafkaApis. We keep the `throttleTimeMs` field
since we intend to enable throttling in the future (especially relevant when we switch to the
built-in quorum mode).

Reviewers: David Arthur <mumrah@gmail.com>
@ijuma

ijuma commented Dec 15, 2020

Copy link
Copy Markdown
Member Author

Merged to trunk and 2.7 branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants