-
Notifications
You must be signed in to change notification settings - Fork 86
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
[server] Ingestion Debugging Improvement by making logging more informative. #1347
base: main
Are you sure you want to change the base?
Conversation
554406f
to
3d6bca3
Compare
3d6bca3
to
03c970a
Compare
StringBuilder sb = new StringBuilder(); | ||
for (Map.Entry<PubSubTopicPartition, TopicPartitionIngestionInfo> entry: topicPartitionIngestionInfoMap | ||
.entrySet()) { | ||
sb.append(entry.getKey().toString()).append(": ").append(entry.getValue().toString()).append("\n"); |
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.
BTW, how big this log could be for a multi-tenant cluster? And how often would this log be printed?
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.
This is controller by:
(!REDUNDANT_LOGGING_FILTER.isRedundantException(slowestTaskIdString)) {
Once per minute.
Map<PubSubTopicPartition, TopicPartitionIngestionInfo> topicPartitionIngestionInfoMap = | ||
getIngestionInfoFromConsumer(consumer); | ||
// Convert Map of ingestion info for this consumer to String for logging with each partition line by line | ||
StringBuilder sb = new StringBuilder(); |
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.
Nowaday's JVM can easily optimize string concatenate by simply using a stringBuilder for you. So String concatenation is not a sin anymore.
Not an issue. Just an FYI.
Summary, imperative, start upper case, don't end with a period
How was this PR tested?
Does this PR introduce any user-facing changes?