-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Log a small subset of segments to refresh for debugging Coordinator refresh logic #16998
Conversation
@Override | ||
void logSegmentsToRefresh(String dataSource, Set<SegmentId> ids) | ||
{ | ||
log.info("Refreshing segments [%s] for datasource [%s]", Iterables.limit(ids, 10), dataSource); |
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 log line should mention that this is a sample of the over all refresh segments.
Also this would be per data source no ? Should we just log 5 segments 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.
Yes, this is per datasource. Sure, I can make it log 5 segment ids.
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.
How frequently will this method be called?
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.
In the worst case this would be called every minute. Generally there aren't segments to be refreshed in each cycle.
Also, as I mentioned in the description we expect this set to be ideally empty on the Coordinator.
@@ -805,6 +808,11 @@ public Set<SegmentId> refreshSegmentsForDataSource(final String dataSource, fina | |||
return retVal; | |||
} | |||
|
|||
void logSegmentsToRefresh(String dataSource, Set<SegmentId> ids) |
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.
Could you add java docs to this method. Also there should be a abstract method called logRefreshSegments and that should return a boolean.
Then each impl can set true or false
So CSMC can return true where as the broker one can return false.
And the log.info call is in the abstract class itself.
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.
What is the benefit of doing that? Also, is there a reason behind making it abstract?
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.
So the main logic of logging does not leak out to impls.
So a rouge impl cannot mess up the logs of the coordinator/broker ever . Atleast that is what I was thinking.
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.
Okay, but is there a reason behind making it abstract and implementing it in both Broker and Coordinator?
The base method could return false and the overridden method in Coordinator could return true?
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.
I feel that the implementation logic for logging the segments should reside in the child classes. This gives flexibility to both Broker and Coordinator to log different number of segments, blacklist/whitelist datasources etc.
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.
The base method could return false and the overridden method in Coordinator could return true?
Sure.
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.
The base method could return false and the overridden method in Coordinator could return true?
Sure.
I haven't made this change yet. Let me know what do you think about this,
I feel that the implementation logic for logging the segments should reside in the child classes. This gives flexibility to both Broker and Coordinator to log different number of segments, blacklist/whitelist datasources etc.
…efresh logic (apache#16998) * Log a small number of segments to refresh per datasource in the Coordinator * review comments * Update log message
With CDS enabled Coordinator shouldn't ideally be issuing metadata queries to refresh segments.
This change logs a subset of segment ids to be refreshed to enable debugging.