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.
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.
Yes, this is per datasource. Sure, I can make it log 5 segment ids.
There was a problem hiding this comment.
How frequently will this method be called?
There was a problem hiding this comment.
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.
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.
What is the benefit of doing that? Also, is there a reason behind making it abstract?
There was a problem hiding this comment.
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.
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.
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.
The base method could return false and the overridden method in Coordinator could return true?
Sure.
There was a problem hiding this comment.
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.