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: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org