clintropolis commented on a change in pull request #7653: Refactor 
SQLMetadataSegmentManager; Change contract of REST methods in 
DataSourcesResource
URL: https://github.com/apache/incubator-druid/pull/7653#discussion_r305119949
 
 

 ##########
 File path: 
server/src/main/java/org/apache/druid/server/http/MetadataResource.java
 ##########
 @@ -129,74 +129,61 @@ public Response getDatabaseDataSources(
     }
   }
 
-  @GET
-  @Path("/datasources/{dataSourceName}")
-  @Produces(MediaType.APPLICATION_JSON)
-  @ResourceFilters(DatasourceResourceFilter.class)
-  public Response getDatabaseSegmentDataSource(@PathParam("dataSourceName") 
final String dataSourceName)
-  {
-    ImmutableDruidDataSource dataSource = 
metadataSegmentManager.getDataSource(dataSourceName);
-    if (dataSource == null) {
-      return Response.status(Response.Status.NOT_FOUND).build();
-    }
-
-    return Response.status(Response.Status.OK).entity(dataSource).build();
-  }
-
   @GET
   @Path("/segments")
   @Produces(MediaType.APPLICATION_JSON)
-  public Response getDatabaseSegments(
+  public Response getAllUsedSegments(
       @Context final HttpServletRequest req,
-      @QueryParam("datasources") final Set<String> datasources,
+      @QueryParam("datasources") final @Nullable Set<String> dataSources,
       @QueryParam("includeOvershadowedStatus") final String 
includeOvershadowedStatus
   )
   {
-    // If we haven't polled the metadata store yet, use an empty list of 
datasources.
-    Collection<ImmutableDruidDataSource> druidDataSources = 
Optional.ofNullable(metadataSegmentManager.getDataSources())
-                                                                    
.orElse(Collections.emptyList());
-    Stream<ImmutableDruidDataSource> dataSourceStream = 
druidDataSources.stream();
-    if (datasources != null && !datasources.isEmpty()) {
-      dataSourceStream = dataSourceStream.filter(src -> 
datasources.contains(src.getName()));
+    if (includeOvershadowedStatus != null) {
+      return getAllUsedSegmentsWithOvershadowedStatus(req, dataSources);
     }
-    final Stream<DataSegment> metadataSegments = dataSourceStream.flatMap(t -> 
t.getSegments().stream());
 
-    if (includeOvershadowedStatus != null) {
-      final Iterable<SegmentWithOvershadowedStatus> authorizedSegments =
-          findAuthorizedSegmentWithOvershadowedStatus(
-              req,
-              metadataSegments
-          );
-      Response.ResponseBuilder builder = Response.status(Response.Status.OK);
-      return builder.entity(authorizedSegments).build();
-    } else {
+    Collection<ImmutableDruidDataSource> dataSourcesWithUsedSegments =
+        segmentsMetadata.getImmutableDataSourcesWithAllUsedSegments();
+    if (dataSources != null && !dataSources.isEmpty()) {
+      dataSourcesWithUsedSegments = dataSourcesWithUsedSegments
+          .stream()
+          .filter(dataSourceWithUsedSegments -> 
dataSources.contains(dataSourceWithUsedSegments.getName()))
+          .collect(Collectors.toList());
+    }
+    final Stream<DataSegment> usedSegments = dataSourcesWithUsedSegments
+        .stream()
+        .flatMap(t -> t.getSegments().stream());
 
-      final Function<DataSegment, Iterable<ResourceAction>> raGenerator = 
segment -> Collections.singletonList(
-          
AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(segment.getDataSource()));
+    final Function<DataSegment, Iterable<ResourceAction>> raGenerator = 
segment -> Collections.singletonList(
+        
AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(segment.getDataSource()));
 
-      final Iterable<DataSegment> authorizedSegments = 
AuthorizationUtils.filterAuthorizedResources(
-          req,
-          metadataSegments::iterator,
-          raGenerator,
-          authorizerMapper
-      );
+    final Iterable<DataSegment> authorizedSegments =
+        AuthorizationUtils.filterAuthorizedResources(req, 
usedSegments::iterator, raGenerator, authorizerMapper);
 
-      Response.ResponseBuilder builder = Response.status(Response.Status.OK);
-      return builder.entity(authorizedSegments).build();
-    }
+    Response.ResponseBuilder builder = Response.status(Response.Status.OK);
+    return builder.entity(authorizedSegments).build();
   }
 
-  private Iterable<SegmentWithOvershadowedStatus> 
findAuthorizedSegmentWithOvershadowedStatus(
+  private Response getAllUsedSegmentsWithOvershadowedStatus(
       HttpServletRequest req,
-      Stream<DataSegment> metadataSegments
+      @Nullable Set<String> dataSources
   )
   {
-    // If metadata store hasn't been polled yet, use empty overshadowed list
-    final Set<SegmentId> overshadowedSegments = Optional
-        .ofNullable(metadataSegmentManager.getOvershadowedSegments())
-        .orElse(Collections.emptySet());
+    DataSourcesSnapshot dataSourcesSnapshot = 
segmentsMetadata.getSnapshotOfDataSourcesWithAllUsedSegments();
 
 Review comment:
   With an empty database that has no segments I'm getting a null pointer 
exception because the `dataSourceSnapshot` set here is `null`.
   
   Allowing an empty database to still set a snapshot, by not returning 
[here](https://github.com/apache/incubator-druid/pull/7653/files#diff-f026a18ff9b149173f87751a695eb06aR955)
 and instead making an 'empty' snapshot remedies this issue, but I'm not sure 
if it is the _right_ fix because it's different than previous behavior, where a 
null would be retained until segments existed.

----------------------------------------------------------------
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]

Reply via email to