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-15435 Fix counts in MigrationManifest #14342
KAFKA-15435 Fix counts in MigrationManifest #14342
Conversation
@@ -60,7 +61,8 @@ public MigrationManifest build() { | |||
if (endTimeNanos == 0) { | |||
endTimeNanos = time.nanoseconds(); | |||
} | |||
return new MigrationManifest(total, batches, endTimeNanos - startTimeNanos, counts); | |||
Map<MetadataRecordType, Integer> orderedCounts = new TreeMap<>(counts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq: why we need a treemap here? Is the sorted order help here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting into a treemap will order the map according to the keys. The effect of this is that the log message becomes deterministic, which is useful for testing. It's arguably also a bit nicer for end users if the output of the message is consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Reviewers: Liu Zeyu <zeyu.luke@gmail.com>, Colin P. McCabe <cmccabe@apache.org>
Reviewers: Liu Zeyu <zeyu.luke@gmail.com>, Colin P. McCabe <cmccabe@apache.org>
This patch fixes an issue in MigrationManifest where we weren't computing the record counts properly. A unit test was added to verify the fix.
This patch also changes the counters in MigrationManifest to use an ordered map so we'll get deterministic output.