Skip to content

MINOR: Eliminate unnecessary map operations in RecordAccumulator.isMuted#7193

Merged
ijuma merged 6 commits into
apache:trunkfrom
lbradstreet:producer-is-muted-optimization
Aug 11, 2019
Merged

MINOR: Eliminate unnecessary map operations in RecordAccumulator.isMuted#7193
ijuma merged 6 commits into
apache:trunkfrom
lbradstreet:producer-is-muted-optimization

Conversation

@lbradstreet

@lbradstreet lbradstreet commented Aug 11, 2019

Copy link
Copy Markdown
Contributor

Avoids calling both containsKey and get from isMuted, when only a single get is necessary.
Also avoid calling remove unless necessary. This could be a reduction of map operations
from 3 to 1.

isMuted showed up as a hotspot in profiling when using the producer with high numbers of partitions.

@ijuma ijuma 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. Worth adding a comment explaining that this method is perf sensitive when there are a large number of partitions.

@ijuma

ijuma commented Aug 11, 2019

Copy link
Copy Markdown
Member

One unrelated test failure in one job and 2 jobs that timed out.

@ijuma

ijuma commented Aug 11, 2019

Copy link
Copy Markdown
Member

retest this please

@ijuma ijuma changed the title MINOR: avoid double HashMap lookup in Producer RecordAccumulator MINOR: Eliminate unnecessary map look ups in RecordAccumulator.isMuted Aug 11, 2019
@ijuma

ijuma commented Aug 11, 2019

Copy link
Copy Markdown
Member

I updated the PR description as the changes could reduce the look-ups from 3 to 1 in the case where the partition is unmuted.

@ijuma ijuma changed the title MINOR: Eliminate unnecessary map look ups in RecordAccumulator.isMuted MINOR: Eliminate unnecessary map operations in RecordAccumulator.isMuted Aug 11, 2019
@ijuma

ijuma commented Aug 11, 2019

Copy link
Copy Markdown
Member

Two jobs passed, one failed with flake kafka.api.ConsumerBounceTest.testClose. Merging to master.

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