surekhasaharan commented on a change in pull request #7595: Optimize
overshadowed segments computation
URL: https://github.com/apache/incubator-druid/pull/7595#discussion_r290439976
##########
File path:
server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java
##########
@@ -757,21 +715,23 @@ public DataSegment map(int index, ResultSet r,
StatementContext ctx) throws SQLE
.addSegmentIfAbsent(segment);
});
-
- replaceDataSourcesSnapshot(newDataSources.entrySet()
- .stream()
- .collect(Collectors.toMap(
- e -> e.getKey(),
- e ->
e.getValue().toImmutableDruidDataSource()
- )));
- }
-
- /**
- * replace "dataSourcesSnapshot" atomically
- */
- private void replaceDataSourcesSnapshot(Map<String,
ImmutableDruidDataSource> dataSources)
- {
- dataSourcesSnapshot = new DataSourcesSnapshot(dataSources);
+ /**
+ * dataSourcesSnapshot is updated only here, please note that if
datasources or segments are enabled or disabled
+ * outside of poll, the dataSourcesSnapshot can become invalid until the
next poll cycle.
+ * {@link DataSourcesSnapshot} computes the overshadowed segments, which
makes it an expensive operation if the
+ * snapshot is invalidated on each segment removal, especially if a user
issues a lot of single segment remove
+ * calls in rapid succession. So the snapshot update is not done outside
of poll at this time.
+ * Updates outside of poll(), were primarily for the user experience, so
users would immediately see the effect of
+ * a segment remove call reflected in MetadataResource API calls. These
updates outside of schecduled poll may be
+ * added back in removeDataSource and removeSegment methods after the
on-demand polling changes from
+ * https://github.com/apache/incubator-druid/pull/7653 are in.
+ */
+ dataSourcesSnapshot = new DataSourcesSnapshot(newDataSources.entrySet()
Review comment:
hmm, i don't see any difference in formatting even if i take the argument
out, it'd be like
```
Map<String, ImmutableDruidDataSource> updatedDataSources =
newDataSources.entrySet()
.stream()
.collect(Collectors.toMap(
e -> e.getKey(),
e -> e.getValue()
.toImmutableDruidDataSource()
));
dataSourcesSnapshot = new DataSourcesSnapshot(updatedDataSources);
```
And I don't see the point of creating this map, since it's only used as
argument to `DataSourcesSnapshot()`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
