Skip to content
Closed
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,12 @@ private Map<TopicPartition, List<ConsumerRecord<K, V>>> pollOnce(long timeout) {

// init any new fetches (won't resend pending fetches)
Cluster cluster = this.metadata.fetch();
Map<TopicPartition, List<ConsumerRecord<K, V>>> records = fetcher.fetchedRecords();
// Avoid block waiting for response if we already have data available, e.g. from another API call to commit.
if (!records.isEmpty()) {
client.poll(0);
return records;
}
fetcher.initFetches(cluster);
client.poll(timeout);
return fetcher.fetchedRecords();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.kafka.common.TopicPartition;
import org.apache.kafka.common.errors.DisconnectException;
import org.apache.kafka.common.errors.InvalidMetadataException;
import org.apache.kafka.common.errors.OffsetOutOfRangeException;
import org.apache.kafka.common.metrics.Metrics;
import org.apache.kafka.common.metrics.Sensor;
import org.apache.kafka.common.metrics.stats.Avg;
Expand Down Expand Up @@ -80,6 +81,8 @@ public class Fetcher<K, V> {
private final Deserializer<K> keyDeserializer;
private final Deserializer<V> valueDeserializer;

private Map<TopicPartition, Long> offsetOutOfRangePartitions;

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.

Should be final?


public Fetcher(ConsumerNetworkClient client,
int minBytes,
int maxWaitMs,
Expand Down Expand Up @@ -108,6 +111,7 @@ public Fetcher(ConsumerNetworkClient client,
this.valueDeserializer = valueDeserializer;

this.records = new LinkedList<PartitionRecords<K, V>>();
this.offsetOutOfRangePartitions = new HashMap<>();

this.sensors = new FetchManagerMetrics(metrics, metricGrpPrefix, metricTags);
this.retryBackoffMs = retryBackoffMs;
Expand Down Expand Up @@ -139,6 +143,7 @@ public void onFailure(RuntimeException e) {
/**
* Update the fetch positions for the provided partitions.
* @param partitions
* @throws NoOffsetForPartitionException If no offset is stored for a given partition and no reset policy is available
*/
public void updateFetchPositions(Set<TopicPartition> partitions) {
// reset the fetch position to the committed position
Expand Down Expand Up @@ -252,16 +257,43 @@ private long listOffset(TopicPartition partition, long timestamp) {
}
}

/**
* If any partition from previous fetchResponse contains OffsetOutOfRange error, throw
* OffsetOutOfRangeException and clear the partition list.
*
* @throws OffsetOutOfRangeException If there is OffsetOutOfRange error in fetchResponse
*/
private void throwIfOffsetOutOfRange() throws OffsetOutOfRangeException {
Map<TopicPartition, Long> currentOutOfRangePartitions = new HashMap<>();

// filter offsetOutOfRangePartitions to retain only the fetchable partitions
for (Map.Entry<TopicPartition, Long> entry: this.offsetOutOfRangePartitions.entrySet()) {
if (!subscriptions.isFetchable(entry.getKey())) {
log.debug("Ignoring fetched records for {} since it is no longer fetchable", entry.getKey());
continue;
}
Long consumed = subscriptions.consumed(entry.getKey());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add some comments here for the checking of the consumed position?

if (consumed != null && entry.getValue() == consumed)
currentOutOfRangePartitions.put(entry.getKey(), entry.getValue());
}
this.offsetOutOfRangePartitions.clear();
if (!currentOutOfRangePartitions.isEmpty())
throw new OffsetOutOfRangeException(currentOutOfRangePartitions);
}

/**
* Return the fetched records, empty the record buffer and update the consumed position.
*
* @return The fetched records per partition
* @throws OffsetOutOfRangeException If there is OffsetOutOfRange error in fetchResponse
*/
public Map<TopicPartition, List<ConsumerRecord<K, V>>> fetchedRecords() {
if (this.subscriptions.partitionAssignmentNeeded()) {
return Collections.emptyMap();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we clear the exception here? If not, then we'll have the exception thrown after the rebalance.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we should not clear exception, because it should be handled in the same way we handle Fetcher.records -- we will still return already fetched records, instead of clearing it, after the rebalance. What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm... In that case, we should check that the out of range partitions are still assigned when the exception is thrown, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, good point! I missed it. Actually this is a good reason why we should store map first and create exception later. Let me update the patch.

} else {
Map<TopicPartition, List<ConsumerRecord<K, V>>> drained = new HashMap<>();
throwIfOffsetOutOfRange();

for (PartitionRecords<K, V> part : this.records) {
if (!subscriptions.isFetchable(part.partition)) {
log.debug("Ignoring fetched records for {} since it is no longer fetchable", part.partition);
Expand Down Expand Up @@ -373,8 +405,11 @@ private Map<Node, FetchRequest> createFetchRequests(Cluster cluster) {
fetchable.put(node, fetch);
}

long offset = this.subscriptions.fetched(partition);
fetch.put(partition, new FetchRequest.PartitionData(offset, this.fetchSize));
long fetched = this.subscriptions.fetched(partition);
long consumed = this.subscriptions.consumed(partition);
// Only fetch data for partitions whose previously fetched data has been consumed
if (consumed == fetched)
fetch.put(partition, new FetchRequest.PartitionData(fetched, this.fetchSize));
}
}

Expand Down Expand Up @@ -442,9 +477,12 @@ private void handleFetchResponse(ClientResponse resp, FetchRequest request) {
|| partition.errorCode == Errors.UNKNOWN_TOPIC_OR_PARTITION.code()) {
this.metadata.requestUpdate();
} else if (partition.errorCode == Errors.OFFSET_OUT_OF_RANGE.code()) {
// TODO: this could be optimized by grouping all out-of-range partitions
long fetchOffset = request.fetchData().get(tp).offset;
if (subscriptions.hasDefaultOffsetResetPolicy())
subscriptions.needOffsetReset(tp);
else
this.offsetOutOfRangePartitions.put(tp, fetchOffset);
log.info("Fetch offset {} is out of range, resetting offset", subscriptions.fetched(tp));
subscriptions.needOffsetReset(tp);
} else if (partition.errorCode == Errors.UNKNOWN.code()) {
log.warn("Unknown error fetching data for topic-partition {}", tp);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,10 @@ public void needOffsetReset(TopicPartition partition) {
needOffsetReset(partition, defaultResetStrategy);
}

public boolean hasDefaultOffsetResetPolicy() {
return defaultResetStrategy != OffsetResetStrategy.NONE;
}

public boolean isOffsetResetNeeded(TopicPartition partition) {
return assignedState(partition).awaitingReset;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,25 @@
*/
package org.apache.kafka.common.errors;

import org.apache.kafka.common.TopicPartition;
import java.util.Map;

/**
* This offset is either larger or smaller than the range of offsets the server has for the given partition.
*
*/
public class OffsetOutOfRangeException extends RetriableException {

private static final long serialVersionUID = 1L;
Map<TopicPartition, Long> offsetOutOfRangePartitions = null;

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.

Should be private and final probably.


public OffsetOutOfRangeException() {
}

public OffsetOutOfRangeException(Map<TopicPartition, Long> offsetOutOfRangePartitions) {
this.offsetOutOfRangePartitions = offsetOutOfRangePartitions;
}

public OffsetOutOfRangeException(String message) {
super(message);
}
Expand All @@ -35,4 +43,8 @@ public OffsetOutOfRangeException(String message, Throwable cause) {
super(message, cause);
}

public Map<TopicPartition, Long> getOutOfRangePartitions() {

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.

In Kafka, we tend to write the method name without the get, ie outOfRangePartitions(). The field name should have the same name though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ijuma Thanks much for your review. Will pay attention to final, private and get method name in the future patch.

Please see updated patch. I didn't add final to variable offsetOutOfRangePartitions in class OffsetOutOfRangeException because otherwise I need to add 4 additional lines offsetOutOfRangePartitions=null to 4 constructor method. This seems redundant. Can you let me know if you prefer me adding final keyword and the additional 4 lines?

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.

Thanks @lindong28.

Regarding the final field in OffsetOutOfRangeException, it's a good question as there is no nice solution. :) Ideally, we would not have 4 constructors since only 1 or 2 tend to be used, but too late to change that now. I think it's OK to leave it as you have as I don't know how committers feel about this particular issue (and I don't want to steer you towards a direction that they may disagre with!).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks @ijuma for your review!

return offsetOutOfRangePartitions;
}

}