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

KAFKA-9686 MockConsumer#endOffsets should be idempotent #8255

Merged
merged 1 commit into from
Mar 10, 2020

Conversation

chia7712
Copy link
Contributor

@chia7712 chia7712 commented Mar 9, 2020

    private Long getEndOffset(List<Long> offsets) {
        if (offsets == null || offsets.isEmpty()) {
            return null;
        }
        return offsets.size() > 1 ? offsets.remove(0) : offsets.get(0);
    }

The above code has two issues.

  1. It does not return the latest offset since the latest offset is at the end of offsets
  2. It removes the element from offsets so MockConsumer#endOffsets gets non-idempotent

https://issues.apache.org/jira/browse/KAFKA-9686

The following flaky is fixed by this PR

  1. KafkaBasedLogTest.testSendAndReadToEnd

Committer Checklist (excluded from commit message)

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

@chia7712
Copy link
Contributor Author

chia7712 commented Mar 9, 2020

retest this please

}
return offsets.size() > 1 ? offsets.remove(0) : offsets.get(0);
if (offsets == null || offsets.isEmpty()) return null;
else return offsets.get(offsets.size() - 1);

Choose a reason for hiding this comment

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

The code seems intentionally written to store a sequence of end offsets so that each call advances the end offset. If we do not need that capability, then perhaps we can change the type of endOffsets and simplify the usage.

cc @bbejeck Looks like you added this logic. Do you know if we rely on it?

Copy link
Contributor

@kkonstantine kkonstantine Mar 10, 2020

Choose a reason for hiding this comment

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

I agree with @hachikuji 's observation. This test broke when #8220 retired the innerUpdateEndOffsets and replaced the two public methods addEndOffsets and updateEndOffsets that were both using innerUpdateEndOffsets with a single updateEndOffsets. However the version that was kept was actually what the unused addEndOffsets was doing (appending to the end offsets) and not what updateEndOffsets described as an overwrite of the value in endOffsets.

Thus, probably the issue should be fixed in updateEndOffsets to keep meaning overwrite. Unless we find a usage for addEndOffsets, in which case we might need some more adjustments too. Of course, if we keep only updateEndOffsets that overwrites the value of the map, we might as well simplify getEndOffset. I haven't tracked when and why addEndOffsets was stopped being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hachikuji @kkonstantine thanks for reviews.

Will address your comment :)

Copy link
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

Added also a comment inline, after tracking that this test broke with a consolidation in: #8220

}
return offsets.size() > 1 ? offsets.remove(0) : offsets.get(0);
if (offsets == null || offsets.isEmpty()) return null;
else return offsets.get(offsets.size() - 1);
Copy link
Contributor

@kkonstantine kkonstantine Mar 10, 2020

Choose a reason for hiding this comment

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

I agree with @hachikuji 's observation. This test broke when #8220 retired the innerUpdateEndOffsets and replaced the two public methods addEndOffsets and updateEndOffsets that were both using innerUpdateEndOffsets with a single updateEndOffsets. However the version that was kept was actually what the unused addEndOffsets was doing (appending to the end offsets) and not what updateEndOffsets described as an overwrite of the value in endOffsets.

Thus, probably the issue should be fixed in updateEndOffsets to keep meaning overwrite. Unless we find a usage for addEndOffsets, in which case we might need some more adjustments too. Of course, if we keep only updateEndOffsets that overwrites the value of the map, we might as well simplify getEndOffset. I haven't tracked when and why addEndOffsets was stopped being used.

Copy link
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

This LGTM now.
Thanks for addressing this @chia7712 !

I'm merging to trunk

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