Skip to content

[LI-HOTFIX] Adding flag to control whether to turn on the async replica fetcher threads#144

Merged
gitlw merged 7 commits into
linkedin:2.4-lifrom
gitlw:flag_for_async_fetcher_renaming_to_async
Apr 28, 2021
Merged

[LI-HOTFIX] Adding flag to control whether to turn on the async replica fetcher threads#144
gitlw merged 7 commits into
linkedin:2.4-lifrom
gitlw:flag_for_async_fetcher_renaming_to_async

Conversation

@gitlw

@gitlw gitlw commented Apr 26, 2021

Copy link
Copy Markdown

TICKET = N/A
LI_DESCRIPTION =
This PR adds a new config li.async.fetcher.enable to control whether the event-based fetcher threads should be used.
It includes the following changes

  1. Reviving the old behavior by bringing back the classes AbstractFetcherManager, ReplicaFetcherManager, and ReplicaFetcherThread. These classes require little review as they are directly checked out from a git snapshot.
  2. Choose either the asyncReplicaFetcherManager or the replicaFetcherManager based on the new config li.async.fetcher.enable

EXIT_CRITERIA = N/A

Committer Checklist (excluded from commit message)

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

@gitlw gitlw requested review from ambroff and xiowu0 April 27, 2021 17:29
@xiowu0

xiowu0 commented Apr 27, 2021

Copy link
Copy Markdown

Should we bring back JBOD-related tests/changes with the new flag?

@gitlw

gitlw commented Apr 27, 2021

Copy link
Copy Markdown
Author

@xiowu0 Thanks for the review. I'm a bit reluctant to bring them back since they don't work when the flag is turned on anyway.

Comment thread core/src/test/scala/unit/kafka/utils/ReplicationUtilsTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala Outdated
@gitlw gitlw force-pushed the flag_for_async_fetcher_renaming_to_async branch from f580c01 to 1e4b67c Compare April 27, 2021 23:42
@gitlw gitlw force-pushed the flag_for_async_fetcher_renaming_to_async branch from 1e4b67c to c0b1c23 Compare April 27, 2021 23:47
Comment thread core/src/main/scala/kafka/server/ReplicaManager.scala Outdated
@gitlw

gitlw commented Apr 28, 2021

Copy link
Copy Markdown
Author

The test testIllegalRequiredAcks is failing, but the root cause is not related to the changes in this PR.
The test is being addressed in a separate PR #149

@gitlw gitlw merged commit 742f286 into linkedin:2.4-li Apr 28, 2021
@gitlw gitlw deleted the flag_for_async_fetcher_renaming_to_async branch April 28, 2021 18:18
gitlw added a commit that referenced this pull request Feb 25, 2022
…ca fetcher threads (#143) (#144)

TICKET = N/A
LI_DESCRIPTION =
This PR adds a new config li.async.fetcher.enable to control whether the event-based fetcher threads should be used.
It chooses either the asyncReplicaFetcherManager or the replicaFetcherManager based on the new config li.async.fetcher.enable
EXIT_CRITERIA = N/A
lmr3796 added a commit that referenced this pull request Mar 15, 2022
lmr3796 pushed a commit to lmr3796/kafka that referenced this pull request Mar 25, 2022
…ca fetcher threads (linkedin#143) (linkedin#144)

TICKET = N/A
LI_DESCRIPTION =
This PR adds a new config li.async.fetcher.enable to control whether the event-based fetcher threads should be used.
It chooses either the asyncReplicaFetcherManager or the replicaFetcherManager based on the new config li.async.fetcher.enable
EXIT_CRITERIA = N/A
lmr3796 pushed a commit to lmr3796/kafka that referenced this pull request Jun 2, 2022
…fetcher threads (linkedin#143) (linkedin#144)

TICKET = N/A
LI_DESCRIPTION =
This PR adds a new config li.async.fetcher.enable to control whether the event-based fetcher threads should be used.
It chooses either the asyncReplicaFetcherManager or the replicaFetcherManager based on the new config li.async.fetcher.enable
EXIT_CRITERIA = N/A
earlcoder added a commit that referenced this pull request Apr 28, 2026
The async/event-based replica fetcher series (TransferLeaderManager,
AbstractAsyncFetcher, AsyncReplicaFetcher, FetcherEventBus,
FetcherEventManager — PRs #121/#123/#124/#143/#144/#403/#406) was
removed in the PR #538 squash. The li.async.fetcher.enable config key
was left behind in KafkaConfig as dead surface area: it parsed and
validated, but had no consumers.

Going with audit P1 #6 option (a): retire the feature, remove the dead
config key. Operators with li.async.fetcher.enable=<anything> in their
server.properties may see an 'Unknown configuration' warning at broker
startup; this is harmless — the broker still starts and the value would
have been a no-op anyway.

Removes:
- Defaults.LiAsyncFetcherEnabled
- KafkaConfig.LiAsyncFetcherEnableProp
- The brokerConfigDef.define(...) registration
- KafkaConfig.liAsyncFetcherEnable accessor

Audit option (b) — porting the entire async fetcher series forward
into 3.6-li — was deferred. If the optimization is needed in the
future, it would need to be reimplemented from scratch on top of
upstream's current ReplicaFetcherThread / ReplicaFetcherManager.
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.

3 participants