Skip to content

KAFKA-9731: Disable immediate fetch response for hw propagation if replica selector is not defined - #8607

Merged
ijuma merged 7 commits into
apache:trunkfrom
ijuma:kafka-9731-increased-fetch-rate-hw-propagation
May 5, 2020
Merged

KAFKA-9731: Disable immediate fetch response for hw propagation if replica selector is not defined#8607
ijuma merged 7 commits into
apache:trunkfrom
ijuma:kafka-9731-increased-fetch-rate-hw-propagation

Conversation

@ijuma

@ijuma ijuma commented May 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

In the case described in the JIRA, there was a 50%+ increase in the total fetch request rate in
2.4.0 due to this change.

I included a few additional clean-ups:

  • Simplify findPreferredReadReplica and avoid unnecessary collection copies.
  • Use LongSupplier instead of Supplier<Long> in SubscriptionState to avoid unnecessary boxing.

Added a unit test to ReplicaManagerTest and cleaned up the test class a bit including
consistent usage of Time in MockTimer and other components.

Committer Checklist (excluded from commit message)

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

@ijuma
ijuma requested review from hachikuji and mumrah May 3, 2020 23:36
gwenshap
gwenshap approved these changes May 4, 2020
View reviewed changes

@gwenshap gwenshap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM (after fixing the missing import).
I liked the cleanup to findPreferredReadReplica.

@ijuma
ijuma force-pushed the kafka-9731-increased-fetch-rate-hw-propagation branch from ed93f35 to ae19a90 Compare May 4, 2020 01:16
@ijuma

ijuma commented May 4, 2020

Copy link
Copy Markdown
Member Author

1 job passed, 1 failed due to a spotBugs issue with Scala 2.12 and one failed with an unrelated flaky test failure:

org.apache.kafka.streams.integration.GlobalKTableIntegrationTest.shouldKStreamGlobalKTableLeftJoin

I pushed a fix for the spotBugs issue and will try to add a test verifying this behavior since we don't see to have any.

@ijuma

ijuma commented May 4, 2020

Copy link
Copy Markdown
Member Author

retest this please

mumrah
mumrah approved these changes May 4, 2020
View reviewed changes

@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.

LGTM, thanks @ijuma!

@hachikuji

hachikuji commented May 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

@ijuma Thanks for the patch. I think the change makes sense. Part of me is inclined to drop the logic for the immediate propagation of the high watermark entirely. Instead, we could say propagation is bounded by the max wait time which users could potentially tune for their own needs. Otherwise enabling this feature becomes sort of risky. We just need to be a little careful to ensure that the partition still gets included in the fetch response if there is a high watermark update. I'll open a JIRA and we can discuss this separately.

@ijuma

ijuma commented May 4, 2020

Copy link
Copy Markdown
Member Author

@hachikuji I was thinking the exact same thing. :) I thought this PR could be the simple one we can backport and then we can discuss the larger change separately as you suggested.

@ijuma
ijuma force-pushed the kafka-9731-increased-fetch-rate-hw-propagation branch from 935b27b to 34d70ce Compare May 4, 2020 18:13
@hachikuji

hachikuji commented May 4, 2020

Copy link
Copy Markdown
Contributor

Filed this issue: https://issues.apache.org/jira/browse/KAFKA-9952.

@ijuma

ijuma commented May 4, 2020

Copy link
Copy Markdown
Member Author

I added a test. @mumrah @gwenshap maybe one of you can take a quick look?

@ijuma

ijuma commented May 5, 2020

Copy link
Copy Markdown
Member Author

retest this please

@ijuma

ijuma commented May 5, 2020

Copy link
Copy Markdown
Member Author

1 job passed, the other 2 had unrelated flaky failures:

kafka.integration.MetricsDuringTopicCreationDeletionTest.testMetricsDuringTopicCreateDelete
org.apache.kafka.streams.integration.GlobalKTableIntegrationTest.shouldKStreamGlobalKTableJoin
org.apache.kafka.streams.integration.QueryableStateIntegrationTest.shouldAllowConcurrentAccesses

@gwenshap

gwenshap commented May 5, 2020

Copy link
Copy Markdown
Contributor

The new test looks good and validates the change made in this PR.

LGTM.

@ijuma
ijuma merged commit fbfda2c into apache:trunk May 5, 2020
@ijuma
ijuma deleted the kafka-9731-increased-fetch-rate-hw-propagation branch May 5, 2020 04:38
Kvicii pushed a commit to Kvicii/kafka that referenced this pull request May 7, 2020
yuyang
Merge branch 'trunk' of github.com:apache/kafka into trunk-read
105fb67 
* 'trunk' of github.com:apache/kafka: (87 commits)
  KAFKA-9865: Expose output topic names from TopologyTestDriver (apache#8483)
  MINOR - Increase the number of Trogdor Histogram buckets to 10000 (apache#8627)
  KAFKA-9768: Fix handling of rest.advertised.listener config (apache#8360)
  KAFKA-9419: Fix possible integer overflow in CircularIterator (apache#7950)
  MINOR: Only add 'Data' suffix for generated request/response/header types (apache#8625)
  KAFKA-9947; Ensure proper shutdown of services in `TransactionsBounceTest` (apache#8602)
  KAFKA-6342; Remove unused workaround for JSON parsing of non-escaped strings (apache#8591)
  MINOR: Pass `-release 8` to scalac and upgrade to Gradle 6.4 (apache#8538)
  KAFKA-9946; Partition deletion event should only be sent if deletion was requested in the StopReplica request (apache#8609)
  MINOR: Improve TopologyTestDriver JavaDocs (apache#8619)
  MINOR: MockAdminClient should return InvalidReplicationFactorException if brokers.size < replicationFactor
  KAFKA-9748: Add Streams eos-beta integration test (apache#8496)
  KAFKA-9731: Disable immediate fetch response for hw propagation if replica selector is not defined (apache#8607)
  HOTFIX: set correct numIterations in shouldAllowConcurrentAccesses
  MINOR: Clean up some test dependencies on ConfigCommand and TopicCommand (apache#8527)
  KAFKA-9918; SslEngineFactory is NOT closed when channel is closing (apache#8551)
  KAFKA-9798: Send one round synchronously before starting the async producer (apache#8565)
  MINOR: Annotate KafkaAdminClientTest.testAlterClientQuotas() with @test
  KAFKA-9589: Enable testLogAppendTimeNonCompressedV2 and fix bug in helper method (apache#8533)
  MINOR: Use min/max function when possible (apache#8577)
  ...

# Conflicts:
#	core/src/main/scala/kafka/log/Log.scala
#	gradle/dependencies.gradle
#	gradle/wrapper/gradle-wrapper.properties
#	gradlew
jwijgerd pushed a commit to buxapp/kafka that referenced this pull request May 14, 2020
@ijuma @jwijgerd
KAFKA-9731: Disable immediate fetch response for hw propagation if re…
588048a 
…plica selector is not defined (apache#8607)

In the case described in the JIRA, there was a 50%+ increase in the total fetch request rate in
2.4.0 due to this change.

I included a few additional clean-ups:
* Simplify `findPreferredReadReplica` and avoid unnecessary collection copies.
* Use `LongSupplier` instead of `Supplier<Long>` in `SubscriptionState` to avoid unnecessary boxing.

Added a unit test to ReplicaManagerTest and cleaned up the test class a bit including
consistent usage of Time in MockTimer and other components.

Reviewers: Gwen Shapira <gwen@confluent.io>, David Arthur <mumrah@gmail.com>, Jason Gustafson <jason@confluent.io>
@jlisam

jlisam commented Jul 22, 2020

Copy link
Copy Markdown

hello @ijuma, I was wondering if you knew when this fix would be backported. We are currently on 2.5.0 and we are currently experiencing this issue. Thank you

@ijuma

ijuma commented Jul 22, 2020

Copy link
Copy Markdown
Member Author

@jlisam It's too late to be included in 2.5.1 (currently in RC stage), but it will be part of 2.6.0 (also at RC stage). Would you be able to upgrade to 2.6.0?

@jlisam

jlisam commented Jul 22, 2020

Copy link
Copy Markdown

@ijuma thanks for the prompt response. I think we should be able to upgrade to 2.6.0 but ideally a backport would be better given that we have spent some time validating the 2.5.0 cluster performance. Anyhow, thanks for fixing this 😄 !

jwijgerd pushed a commit to buxapp/kafka that referenced this pull request Aug 17, 2020
@ijuma @jwijgerd
KAFKA-9731: Disable immediate fetch response for hw propagation if re…
1541cd2 
…plica selector is not defined (apache#8607)

In the case described in the JIRA, there was a 50%+ increase in the total fetch request rate in
2.4.0 due to this change.

I included a few additional clean-ups:
* Simplify `findPreferredReadReplica` and avoid unnecessary collection copies.
* Use `LongSupplier` instead of `Supplier<Long>` in `SubscriptionState` to avoid unnecessary boxing.

Added a unit test to ReplicaManagerTest and cleaned up the test class a bit including
consistent usage of Time in MockTimer and other components.

Reviewers: Gwen Shapira <gwen@confluent.io>, David Arthur <mumrah@gmail.com>, Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mumrah mumrah mumrah approved these changes
@gwenshap gwenshap gwenshap approved these changes
@hachikuji hachikuji Awaiting requested review from hachikuji

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ijuma @hachikuji @gwenshap @jlisam @mumrah