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

GH-1107: Kafka metrics lag check #1123

Closed
wants to merge 1 commit into from

Conversation

sobychacko
Copy link
Contributor

When KafkaBinderMetrics checks lags for offsets in partitions, if the partitions
have nothing committed yet, it unnecessarily tries to find the last committed offset.
This is causing some unncessary logging. Avoid this by only checking the committed
offsets in a partition, if the current end offset is greater than zero.

Resolves #1107

When KafkaBinderMetrics checks lags for offsets in partitions, if the partitions
have nothing committed yet, it unnecessarily tries to find the last committed offset.
This is causing some unncessary logging. Avoid this by only checking the committed
offsets in a partition, if the current end offset is greater than zero.

Resolves spring-cloud#1107
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

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

I am not sure what's driving this change; when I tested this, I simply got offset=0 for partitions that had no committed offset.

@KafkaListener(id = "kbgh1123", topics = "kbgh1123")
public void listen(String in, Consumer<?, ?> consumer) {
	Map<TopicPartition, OffsetAndMetadata> committed =
			consumer.committed(Set.of(new TopicPartition("kbgh1123", 0), new TopicPartition("kbgh1123", 1)));
	System.out.println(in + "\n" + committed);
}
foo
{kbgh1123-1=OffsetAndMetadata{offset=0, leaderEpoch=null, metadata=''}, kbgh1123-0=OffsetAndMetadata{offset=0, leaderEpoch=null, metadata=''}}

@@ -210,11 +212,16 @@ private long findTotalTopicGroupLag(String topic, String group, Map<String, Cons

for (Map.Entry<TopicPartition, Long> endOffset : endOffsets
.entrySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not get all the committed offsets in a single call here .committed(topicPartitions) rather that one-at-a-time in the loop below?

@garyrussell
Copy link
Contributor

Oh; sorry - I see it's these logs he's complaining about Found no committed offset for partition kbgh1123-1.

I still think it would be better to fetch them in one call; why not advise the user the set the log level for o.a.k.c.c.internals.ConsumerCoordinator to WARN?

@sobychacko
Copy link
Contributor Author

@garyrussell Ya, we can certainly advise the user of that. I saw your comment on the issue. I was thinking that if we already know that the end offset for the partition is 0, then why attempt another committed(...) call for finding the current offset? But now I realize that, in between those checks, if the offsets have advanced, then we will miss those updates. Therefore, I could see this PR as moot. We can close this PR and then wait for the user to confirm if the WARN approach is all they need. What do you think?

@garyrussell
Copy link
Contributor

Agreed. But I would still prefer to see a single call to committed(). It is a blocking call. Perhaps we should open a new issue for that.

@sobychacko
Copy link
Contributor Author

Closing this PR; The above concern is addressed through #1130.

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.

A lot of logs are produced due to metrics
2 participants