Skip to content

MINOR: Remove use of NoSuchElementException#10344

Merged
hachikuji merged 3 commits intoapache:trunkfrom
jsancio:minor-remove-exception
Mar 18, 2021
Merged

MINOR: Remove use of NoSuchElementException#10344
hachikuji merged 3 commits intoapache:trunkfrom
jsancio:minor-remove-exception

Conversation

@jsancio
Copy link
Copy Markdown
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
Comment thread core/src/main/scala/kafka/raft/KafkaMetadataLog.scala Outdated
@hachikuji
Copy link
Copy Markdown
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
Copy Markdown
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