gianm commented on code in PR #17935:
URL: https://github.com/apache/druid/pull/17935#discussion_r2060684725
##########
docs/configuration/index.md:
##########
@@ -925,7 +925,7 @@ These Coordinator static configurations can be defined in
the `coordinator/runti
|--------|-----------|-------|
|`druid.manager.config.pollDuration`|How often the manager polls the config
table for updates.|`PT1M`|
|`druid.manager.segments.pollDuration`|The duration between polls the
Coordinator does for updates to the set of active segments. Generally defines
the amount of lag time it can take for the Coordinator to notice new
segments.|`PT1M`|
-|`druid.manager.segments.useCache`|(Experimental) Denotes the usage mode of
the segment metadata cache. The cache provides a performance improvement over
the polling mechanism currently employed by the Coordinator by retrieving
payloads of only updated segments. Possible cache modes are: (a) `never`: Cache
is disabled. (b) `always`: Reads are always done from the cache. Service
start-up will be blocked until cache has synced with the metadata store at
least once. Transactions will block until cache has synced with the metadata
store at least once after becoming leader. (c) `ifSynced`: Reads are done from
the cache only if it has already synced with the metadata store. This mode does
not block service start-up but blocks read transactions.|`never`|
+|`druid.manager.segments.useCache`|(Experimental) Denotes the usage mode of
the segment metadata incremental cache. This cache provides a performance
improvement over the polling mechanism currently employed by the Coordinator as
it retrieves payloads of only updated segments. Possible cache modes are: (a)
`never`: Incremental cache is disabled. (b) `always`: Incremental cache is
enabled. Service start-up will be blocked until cache has synced with the
metadata store at least once. (c) `ifSynced`: Cache is enabled. This mode does
not block service start-up and is a way to retain existing behaviour of the
Coordinator. If the incremental cache is in modes `always` or `ifSynced`, reads
from the cache will block until it has synced with the metadata store at least
once after becoming leader. The Coordinator never writes to this cache.|`never`|
Review Comment:
behavior (we typically use US spelling)
##########
docs/configuration/index.md:
##########
@@ -925,7 +925,7 @@ These Coordinator static configurations can be defined in
the `coordinator/runti
|--------|-----------|-------|
|`druid.manager.config.pollDuration`|How often the manager polls the config
table for updates.|`PT1M`|
|`druid.manager.segments.pollDuration`|The duration between polls the
Coordinator does for updates to the set of active segments. Generally defines
the amount of lag time it can take for the Coordinator to notice new
segments.|`PT1M`|
-|`druid.manager.segments.useCache`|(Experimental) Denotes the usage mode of
the segment metadata cache. The cache provides a performance improvement over
the polling mechanism currently employed by the Coordinator by retrieving
payloads of only updated segments. Possible cache modes are: (a) `never`: Cache
is disabled. (b) `always`: Reads are always done from the cache. Service
start-up will be blocked until cache has synced with the metadata store at
least once. Transactions will block until cache has synced with the metadata
store at least once after becoming leader. (c) `ifSynced`: Reads are done from
the cache only if it has already synced with the metadata store. This mode does
not block service start-up but blocks read transactions.|`never`|
+|`druid.manager.segments.useCache`|(Experimental) Denotes the usage mode of
the segment metadata incremental cache. This cache provides a performance
improvement over the polling mechanism currently employed by the Coordinator as
it retrieves payloads of only updated segments. Possible cache modes are: (a)
`never`: Incremental cache is disabled. (b) `always`: Incremental cache is
enabled. Service start-up will be blocked until cache has synced with the
metadata store at least once. (c) `ifSynced`: Cache is enabled. This mode does
not block service start-up and is a way to retain existing behaviour of the
Coordinator. If the incremental cache is in modes `always` or `ifSynced`, reads
from the cache will block until it has synced with the metadata store at least
once after becoming leader. The Coordinator never writes to this cache.|`never`|
Review Comment:
should be `useIncrementalCache`
##########
server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java:
##########
@@ -84,16 +85,21 @@ public SqlSegmentsMetadataManagerV2(
this.managerConfig = managerConfig.get();
this.segmentMetadataCache = segmentMetadataCache;
this.schemaConfig = centralizedDatasourceSchemaConfig;
+
+ // Segment metadata cache currently cannot handle schema updates
+ if (segmentMetadataCache.isEnabled() && schemaConfig.isEnabled()) {
+ throw InvalidInput.exception(
+ "Segment metadata incremental cache and segment schema cache cannot
be used together."
Review Comment:
another nit: would be nice to include the property names in the error
message, so operators know what properties to search their config files for.
##########
server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentsMetadataManagerV2.java:
##########
@@ -84,16 +85,21 @@ public SqlSegmentsMetadataManagerV2(
this.managerConfig = managerConfig.get();
this.segmentMetadataCache = segmentMetadataCache;
this.schemaConfig = centralizedDatasourceSchemaConfig;
+
+ // Segment metadata cache currently cannot handle schema updates
+ if (segmentMetadataCache.isEnabled() && schemaConfig.isEnabled()) {
+ throw InvalidInput.exception(
+ "Segment metadata incremental cache and segment schema cache cannot
be used together."
Review Comment:
nit: persona for this error message should be `OPERATOR`, but `InvalidInput`
uses persona `USER`. I think it doesn't really matter much though, since this
error isn't going to go out on an HTTP API, so the special DruidException
persona and category stuff won't be used. It could just as well be a regular
`IllegalArgumentException` or `UnsupportedOperationException`.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordDataSourcesResource.java:
##########
@@ -213,11 +209,21 @@
@ResourceFilters(DatasourceResourceFilter.class)
public Response markSegmentAsUsed(
@PathParam("dataSourceName") String dataSourceName,
- @PathParam("segmentId") String segmentId
+ @PathParam("segmentId") String serializedSegmentId
)
{
+ final SegmentId segmentId = SegmentId.tryParse(dataSourceName,
serializedSegmentId);
+ if (segmentId == null) {
+ return Response.status(Response.Status.BAD_REQUEST).entity(
+ StringUtils.format(
+ "Could not parse Segment ID[%s] for DataSource[%s]",
+ serializedSegmentId, dataSourceName
+ )
+ ).build();
Review Comment:
CodeQL has flagged this as a possible XSS vulnerability. Please double check
that it is OK. (For example, this is OK if the response content-type is
`text/plain`.) If it's not a real vulnerability then please dismiss the CodeQL
alert.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
