leventov commented on a change in pull request #7595: Optimize overshadowed
segments computation
URL: https://github.com/apache/incubator-druid/pull/7595#discussion_r290830307
##########
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:
Although it's not formally prohibited by the code style, I prefer fixed
indentation rather than aligned indentation. Aligned indentation forces
excessive line breakups, as in your example. Also, when variables or types are
renamed aligned indentation breaks.
Your code could be
```java
Map<String, ImmutableDruidDataSource> updatedDataSources = newDataSources
.entrySet()
.stream()
.collect(Collectors.toMap(Entry::getKey, e ->
e.getValue().toImmutableDruidDataSource()));
dataSourcesSnapshot = new DataSourcesSnapshot(updatedDataSources);
```
But in this case, it's actually better to extract the first part as a method
`Map<K, V2> mapValues(Map<K, V> map, Function<V, V2> valueMapper)` in
`CollectionUtils` because this boilerplate code appears in several places in
the codebase. Could you please do that?
----------------------------------------------------------------
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]
