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

MINOR: Remove use of NoSuchElementException #10344

Merged
merged 3 commits into from
Mar 18, 2021

Conversation

jsancio
Copy link
Member

@jsancio jsancio commented Mar 17, 2021

Replace the use of the method last and first in ConcurrentSkipListSet with the descending and ascending iterator respectively. The methods last and first throw an exception when the set is empty this causes poor KafkaRaftClient performance when there aren't any snapshots.

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

@jsancio jsancio force-pushed the minor-remove-exception branch from 9ec67a1 to 67196e0 Compare March 17, 2021 23:36
@hachikuji
Copy link
Contributor

hachikuji commented Mar 18, 2021

One other thing maybe we can fix here. In ReplicatedLog.validateOffsetAndEpoch, we have the following check:

        } else if (
                earliestSnapshotId().isPresent() &&
                ((offset < startOffset()) ||
                 (offset == startOffset() && epoch != earliestSnapshotId().get().epoch) ||
                 (epoch < earliestSnapshotId().get().epoch))
        ) {

We should be caching the result of earliestSnapshotId. This is probably one reason it was killing performance.

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM

@hachikuji hachikuji merged commit 6190fb3 into apache:trunk Mar 18, 2021
@jsancio jsancio deleted the minor-remove-exception branch March 18, 2021 18:09
ijuma added a commit to confluentinc/kafka that referenced this pull request Mar 19, 2021
Conflicts:
* build.gradle: keep `dependencySubstitution` Confluent addition in
`resolutionStrategy` and take upstream changes.

Commits:
* apache-github/trunk:
  KAFKA-12503: inform threads to resize their cache instead of doing so for them (apache#10356)
  KAFKA-10697: Remove ProduceResponse.responses (apache#10332)
  MINOR: Exclude KIP-500.md from rat check (apache#10354)
  MINOR: Move `configurations.all` to be a child of `allprojects` (apache#10349)
  MINOR: Remove use of `NoSuchElementException` in `KafkaMetadataLog` (apache#10344)
  MINOR: Start the broker-to-controller channel for request forwarding (apache#10340)
  KAFKA-12382: add a README for KIP-500 (apache#10227)
  MINOR: Fix BaseHashTable sizing (apache#10334)
  KAFKA-10357: Add setup method to internal topics (apache#10317)
  MINOR: remove redundant null check when testing specified type (apache#10314)
  KAFKA-12293: Remove JCenter from buildscript and delete buildscript.gradle
  KAFKA-12491: Make rocksdb an `api` dependency for `streams` (apache#10341)
  KAFKA-12454: Add ERROR logging on kafka-log-dirs when given brokerIds do not  exist in current kafka cluster (apache#10304)
  KAFKA-12459; Use property testing library for raft event simulation tests (apache#10323)
  MINOR: fix failing ZooKeeper system tests (apache#10297)
  MINOR: fix client_compatibility_features_test.py (apache#10292)
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