-
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
Skip refresh for unused segments in metadata cache #16990
Skip refresh for unused segments in metadata cache #16990
Conversation
ImmutableDruidDataSource druidDataSource = | ||
sqlSegmentsMetadataManager.getImmutableDataSourceWithUsedSegments(segmentId.getDataSource()); | ||
|
||
if (druidDataSource != null && druidDataSource.getSegment(segmentId) != null) { |
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.
Can we remove the segment if the segment is unused when we receive a callback from the inventory view.?
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.
Whenever SqlSegmentsMetadataManager
polls the database and returns the latest list of used segments, we should check that list and remove any segments from cache which are not in that list.
This removal should not be triggered based on any callback from server inventory view. Metadata store should be the source of truth on whether a segment is used or not.
Also, I was under the impression that this feature is already included. Can you confirm, @findingrish ?
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.
Whenever SqlSegmentsMetadataManager polls the database and returns the latest list of used segments, we should check that list and remove any segments from cache which are not in that list.
This doesn't happen currently. Callbacks from the inventory view is the source of truth for the metadata cache.
The reason being that segments in the metadata cache are representative of the available segments loaded on various data servers in the cluster.
I feel that this would be an expensive operation as well.
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.
Callbacks from the inventory view is the source of truth for the metadata cache.
I am not sure I understand. Isn't the schema coming from the metadata store?
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 am not sure I understand. Isn't the schema coming from the metadata store?
That is partly correct, we still rely on metadata query to fetch schema for segments published from MSQ job.
The datasource schema is still computed from the segments which are available
.
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, my understanding is that we should be doing the following:
- Every minute (or less), coordinator polls the metadata store to get the list of used segments and their schemas
- Coordinator populates its cache with this information
- Coordinator gets rid of any segment not found in the above list
- If any used segment that the Coordinator received in the latest poll does not have the schema populated, we fire a metadata query to get that schema and populate it back into the DB.
- Subsequent polls will get schema for that segment also.
- If any service requests schema for a used segment, coordinator sends back an OK response
- If any service requests schema for an unused segment, coordinator sends back a NOT FOUND response.
Let me know if this aligns with what we need and what we currently have.
Traditionally, coordinator has been aware of "used" segments only (both overshadowed and non-overshadowed). We should continue to maintain those semantics.
Coordinator does also maintain an inventory view which might show unused segments on some server. But in the immediately next run, the Coordinator unloads the unused segments.
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 don't think we can base the schema building logic entirely on segments polled from the database since realtime segment schema isn't present in the metadata database.
Also, in the present mechanism datasource schema is rebuilt only on segment addition/removal. In the proposed approach the datasource schema would be rebuilt after each poll leading to overhead?
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.
Can we remove the segment if the segment is unused when we receive a callback from the inventory view.?
@cryptoe I think there could be a scenario where this would still fail.
- Segment
s1
got added tofoo
and the cache tries to refresh it. - The cache is building the schema for
foo
and in parallels2
was marked unused. - The caches marks
s2
to be refreshed in the next cycle. - Lets assume that the segment removal callback didn't make it to the Coordinator until the next refresh cycle got triggered.
s2
an unused segment will be refreshed in that cycle.
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 don't think we can base the schema building logic entirely on segments polled from the database since realtime segment schema isn't present in the metadata database.
That's a fair point. How does the coordinator currently distinguish between a realtime segment and a published segment?
Also, in the present mechanism datasource schema is rebuilt only on segment addition/removal. In the proposed approach the datasource schema would be rebuilt after each poll leading to overhead?
Don't we already do that? If not, how is the schema polled from the DB used?
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 does the coordinator currently distinguish between a realtime segment and a published segment?
Both segments are added to the timeline, the difference is in refreshing the segment schema.
Traditionally, a realtime segment would be refreshed every minute by issuing metadata query to tasks serving the segment whereas a published segment would be refreshed only once after the segment has been added.
In the current scheme, schema for published segment is obtained from the database whereas realtime segment schema is periodically pushed to the Coordinator from tasks.
Don't we already do that? If not, how is the schema polled from the DB used?
No we don't do this as of now. The segment schema polled from the database is cached and used on-demand ie. when a segment is added or datasource schema is being rebuilt.
* Skip refresh for unused segments in metadata cache * Cover the condition where a used segment missing schema is marked for refresh * Fix test
* Skip refresh for unused segments in metadata cache * Cover the condition where a used segment missing schema is marked for refresh * Fix test
* Skip refresh for unused segments in metadata cache * Cover the condition where a used segment missing schema is marked for refresh * Fix test
)" This reverts commit b0e07bf.
* Skip refresh for unused segments in metadata cache * Cover the condition where a used segment missing schema is marked for refresh * Fix test
* Skip refresh for unused segments in metadata cache * Cover the condition where a used segment missing schema is marked for refresh * Fix test
Parent issue: #14989
In the metadata cache, unused segment could be marked for refresh leading to unnecessary metadata queries.
One such scenario is:
This PR tries to place a check for unused segments before marking it for refresh.
This PR has: