Copilot commented on code in PR #19180:
URL: https://github.com/apache/pinot/pull/19180#discussion_r3762661796
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutor.java:
##########
@@ -142,6 +141,37 @@ public SegmentConversionResult executeTask(PinotTaskConfig
pinotTaskConfig)
reportTaskProcessingMetrics(tableNameWithType, taskType,
segmentMetadata.getTotalDocs());
}
+ SegmentZKMetadataCustomMapModifier segmentZKMetadataCustomMapModifier =
+ getSegmentZKMetadataCustomMapModifier(pinotTaskConfig,
segmentConversionResult);
+ if
(convertedSegmentDir.getCanonicalFile().equals(indexDir.getCanonicalFile())) {
+ _eventObserver.notifyProgress(_pinotTaskConfig,
+ "Updating ZK metadata without uploading unchanged segment: " +
segmentName);
+ try {
+ SegmentConversionUtils.updateSegmentZKMetadata(tableNameWithType,
segmentName, uploadURL,
+ originalSegmentCrc, segmentZKMetadataCustomMapModifier,
authProvider);
+ LOGGER.info("Updated ZK metadata without uploading unchanged
segment: {} of table: {}", segmentName,
+ tableNameWithType);
+ return segmentConversionResult;
+ } catch (HttpErrorStatusException e) {
+ if (e.getStatusCode() != HttpStatus.SC_NOT_FOUND) {
Review Comment:
Older controllers already expose `GET` on this exact metadata path, so
JAX-RS rejects the new `PUT` with 405 Method Not Allowed rather than 404.
Restricting fallback to 404 therefore breaks the stated mixed-version rollout:
an unchanged conversion fails instead of using the upload path. Treat 405 as an
unavailable endpoint as well.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutorTest.java:
##########
@@ -134,6 +136,38 @@ public void testExecuteTaskSucceedsWhenUploadSucceeds()
}
}
+ @Test
+ public void testExecuteTaskUpdatesMetadataWithoutUploadingUnchangedSegment()
+ throws Exception {
+ try (MockedStatic<SegmentConversionUtils> mocked =
Mockito.mockStatic(SegmentConversionUtils.class)) {
+ TestSingleSegmentConversionExecutor executor = new
TestSingleSegmentConversionExecutor(true);
+ SegmentConversionResult result =
executor.executeTask(createTaskConfig());
+
+ Assert.assertEquals(result.getSegmentName(), SEGMENT_NAME);
+ mocked.verify(() ->
SegmentConversionUtils.updateSegmentZKMetadata(Mockito.eq(TABLE_NAME_WITH_TYPE),
+ Mockito.eq(SEGMENT_NAME), Mockito.anyString(),
Mockito.eq(Long.toString(SEGMENT_CRC)), Mockito.any(),
+ Mockito.any()));
+ mocked.verifyNoMoreInteractions();
+ }
+ }
+
+ @Test
+ public void testExecuteTaskFallsBackToUploadWhenMetadataApiIsUnavailable()
+ throws Exception {
+ try (MockedStatic<SegmentConversionUtils> mocked =
Mockito.mockStatic(SegmentConversionUtils.class)) {
+ mocked.when(() ->
SegmentConversionUtils.updateSegmentZKMetadata(Mockito.anyString(),
Mockito.anyString(),
+ Mockito.anyString(), Mockito.anyString(), Mockito.any(),
Mockito.any()))
+ .thenThrow(new HttpErrorStatusException("metadata API not found",
HttpStatus.SC_NOT_FOUND));
Review Comment:
This mock does not reproduce an older controller: because that controller
already has `GET /segments/{table}/{segment}/metadata`, sending `PUT` returns
405, not 404. Simulate 405 here so the test actually guards the mixed-version
fallback.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -317,6 +322,76 @@ public Map<String, Object> getSegmentMetadata(
}
}
+ @PUT
+ @Path("segments/{tableNameWithType}/{segmentName}/metadata")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableNameWithType",
action = Actions.Table.UPLOAD_SEGMENT)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Update the custom map in the ZK metadata for a
segment",
+ notes = "Updates only the segment ZK metadata custom map without
uploading or refreshing the segment")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 400, message = "Invalid table name, CRC, or custom
map modifier"),
+ @ApiResponse(code = 404, message = "Table or segment not found"),
+ @ApiResponse(code = 409, message = "Segment metadata changed
concurrently"),
+ @ApiResponse(code = 412, message = "Segment CRC does not match")
+ })
+ public SuccessResponse updateSegmentZKMetadataCustomMap(
+ @ApiParam(value = "Table name with type", required = true, example =
"myTable_OFFLINE")
+ @PathParam("tableNameWithType") String tableNameWithType,
+ @ApiParam(value = "Name of the segment", required = true)
@PathParam("segmentName") @Encoded String segmentName,
+ @ApiParam(value = "Expected segment CRC", required = true)
@HeaderParam(HttpHeaders.IF_MATCH)
+ String expectedCrcString,
+ @ApiParam(value = "Custom map modifier", required = true) String
customMapModifierJson,
+ @Context HttpHeaders headers) {
+ tableNameWithType = DatabaseUtils.translateTableName(tableNameWithType,
headers);
+ segmentName = URIUtils.decode(segmentName);
+ if (TableNameBuilder.getTableTypeFromTableName(tableNameWithType) == null)
{
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Table type not provided with table name: %s",
tableNameWithType), Status.BAD_REQUEST);
+ }
+
+ long expectedCrc;
+ try {
+ expectedCrc = Long.parseLong(expectedCrcString);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Missing or invalid
If-Match segment CRC", Status.BAD_REQUEST,
+ e);
+ }
+
+ SegmentZKMetadataCustomMapModifier customMapModifier;
+ try {
+ customMapModifier = new
SegmentZKMetadataCustomMapModifier(customMapModifierJson);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Invalid segment ZK
metadata custom map modifier",
Review Comment:
The modifier constructor does not validate the JSON shape: a scalar or array
`map` is treated as empty, and nested/non-string values are silently coerced
with `asText()`. Such malformed requests therefore return 200 (and can write
unintended values) despite this API documenting invalid modifiers as 400.
Validate that `mapModifyMode` is a valid string enum and `map` is null or an
object containing only string values before applying it, with malformed-shape
tests.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -317,6 +322,76 @@ public Map<String, Object> getSegmentMetadata(
}
}
+ @PUT
+ @Path("segments/{tableNameWithType}/{segmentName}/metadata")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableNameWithType",
action = Actions.Table.UPLOAD_SEGMENT)
Review Comment:
Add `@Authenticate(AccessType.UPDATE)` to this mutating endpoint. With an
`AccessControl` implementation whose `protectAnnotatedOnly()` returns true,
`AuthenticationFilter` returns before both authentication and fine-grained
authorization when `@Authenticate` is absent, leaving this ZK mutation
unprotected; neighboring segment mutation/upload endpoints carry both
annotations.
--
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]
