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

Remove CompletedFetch type from Fetcher #7228

Merged
merged 3 commits into from Sep 6, 2019
Merged

Conversation

@seglo
Copy link
Contributor

@seglo seglo commented Aug 21, 2019

Jira issue KAFKA-8822.

The CompletedFetch type is no longer required due to the work merged in PR #6988 (KAFKA-7548). This PR consolidates CompletedFetch into PartitionRecords as discussed in #6988 (comment) with @hachikuji.

@seglo
Copy link
Contributor Author

@seglo seglo commented Aug 24, 2019

Retest this please.

Loading

@seglo seglo force-pushed the seglo/KAFKA-8822 branch 2 times, most recently from d33d30f to afd1f4c Aug 26, 2019
@hachikuji hachikuji self-assigned this Aug 27, 2019
Copy link
Contributor

@hachikuji hachikuji left a comment

Thanks, nice cleanup! Left a few small comments.

Loading

@@ -1422,8 +1418,9 @@ private void drain() {
}
}

// TODO: this has no usages. remove?
Copy link
Contributor

@hachikuji hachikuji Aug 28, 2019

Choose a reason for hiding this comment

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

Yeah, let's remove if it's unused. We can add back if we need it.

Loading

@@ -1379,10 +1368,12 @@ public static Sensor throttleTimeSensor(Metrics metrics, FetcherMetricsRegistry

private class PartitionRecords {
Copy link
Contributor

@hachikuji hachikuji Aug 28, 2019

Choose a reason for hiding this comment

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

What do you think about taking the name CompletedFetch here. It seemed a little more descriptive.

Loading

Copy link
Contributor Author

@seglo seglo Aug 28, 2019

Choose a reason for hiding this comment

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

I agree, since it's private to the Fetcher anyway. I've renamed it and a few obvious related variables and javadocs. There are some other things that could be renamed too, but I wanted to see what you thought first.

  • CompletedFetch.isFetched - This sounds a little strange now. isFetched == true when the records are drained which normally happens after it's been fully consumed. We could rename it to isDrained or isConsumed.
  • Fetcher.nextInLineRecords - I assume this was derived from the PartitionRecords type as well, but I'm not sure. It could be renamed to nextInLineCompletedFetch or nextInLineFetch.

Loading

Copy link
Contributor

@hachikuji hachikuji Sep 4, 2019

Choose a reason for hiding this comment

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

Sorry, missed this comment. Both of your suggestions sound good to me. I like isConsumed and nextInLineFetch.

Loading

FetchResponse.PartitionData<Records> partitionData,
FetchResponseMetricAggregator metricAggregator,
Iterator<? extends RecordBatch> batches,
Long fetchedOffset,
Copy link
Contributor

@hachikuji hachikuji Aug 28, 2019

Choose a reason for hiding this comment

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

nit: fetchOffset instead of fetchedOffset? I think this is meant to describe the offset from the fetch request.

Loading

@seglo seglo force-pushed the seglo/KAFKA-8822 branch 2 times, most recently from 8cc2e67 to 097d0dc Sep 2, 2019
Copy link
Contributor

@hachikuji hachikuji left a comment

LGTM. Thanks for the patch! I'll rerun tests one more time before merging since it's been a couple weeks.

Loading

@hachikuji
Copy link
Contributor

@hachikuji hachikuji commented Sep 4, 2019

retest this please

Loading

@seglo
Copy link
Contributor Author

@seglo seglo commented Sep 4, 2019

@hachikuji Thanks. There were a couple more things that could be renamed if you want. See this comment I made earlier in the PR: #7228 (comment)

If you have a moment could you take a quick look at my comments in the benchmark PR? #7221

Loading

@hachikuji
Copy link
Contributor

@hachikuji hachikuji commented Sep 4, 2019

@seglo Sorry, missed your comment. Your suggestions sound good to me.

Loading

@seglo seglo force-pushed the seglo/KAFKA-8822 branch from 097d0dc to b28cc84 Sep 5, 2019
@seglo
Copy link
Contributor Author

@seglo seglo commented Sep 5, 2019

Ok, I think it's ready to go. I'll watch to get a clean test run.

Loading

@seglo
Copy link
Contributor Author

@seglo seglo commented Sep 5, 2019

Retest this please.

Loading

@hachikuji hachikuji merged commit ad3ccf8 into apache:trunk Sep 6, 2019
3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants