xiangfu0 commented on code in PR #19231:
URL: https://github.com/apache/pinot/pull/19231#discussion_r3857802589
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -367,19 +358,15 @@ private SuccessResponse uploadSegment(@Nullable String
tableName, TableType tabl
// Fetch segment name
String segmentName = segmentMetadata.getName();
- // Fetch table name. Try to derive the table name from the parameter and
then from segment metadata
- String rawTableName;
- if (StringUtils.isNotEmpty(tableName)) {
- rawTableName = TableNameBuilder.extractRawTableName(tableName);
- } else {
- // TODO: remove this when we completely deprecate the table name from
segment metadata
- rawTableName = segmentMetadata.getTableName();
- LOGGER.warn("Table name is not provided as request query parameter
when uploading segment: {} for table: {}",
- segmentName, rawTableName);
- }
- String tableNameWithType = tableType == TableType.OFFLINE
- ? TableNameBuilder.OFFLINE.tableNameWithType(rawTableName)
- : TableNameBuilder.REALTIME.tableNameWithType(rawTableName);
+ String rawTableName = resolveDestinationTableName(tableName,
tableNameInHeader, segmentMetadata.getTableName(),
+ tableType, headers, requireMatchingMetadataTable);
Review Comment:
Moved request and header normalization to the start of `uploadSegment`,
before temp-file creation, URI download, decryption, or metadata extraction. I
also added a URI-upload regression that asserts the segment source receives
zero requests for an invalid request/header binding.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -830,8 +895,8 @@ private void decryptFile(String crypterClassName, File
tempEncryptedFile, File t
// request if a multipart object is not sent. This endpoint does not move
the segment to its final location;
// it keeps it at the downloadURI header that is set. We will not support
this endpoint going forward.
public void uploadSegmentAsJson(String segmentJsonStr,
- @ApiParam(value = "Name of the table to upload into. Overrides
segment.table.name in segment metadata when set "
- + "(allows promoting a segment built for another table). Falls back
to metadata when omitted.")
+ @ApiParam(value = "Name of the table to upload into. Must match
segment.table.name in segment metadata when both "
+ + "are set. Falls back to metadata when omitted.")
Review Comment:
Kept strict v1 matching because the destination-binding requirement
intentionally rejects inconsistent request and metadata tables. The new
Compatibility section calls out that v1 mismatches return 400 and directs
intentional metadata overrides or promotion to `/v2/segments`.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -735,6 +736,70 @@ private void cleanupTempFiles(List<File> tempFiles) {
}
}
+ @VisibleForTesting
+ static String resolveDestinationTableName(@Nullable String requestTableName,
@Nullable String headerTableName,
+ @Nullable String metadataTableName, TableType tableType, HttpHeaders
headers,
+ boolean requireMatchingMetadataTable) {
+ String normalizedRequestTable = normalizeTableName(requestTableName,
tableType, headers, "request tableName");
+ String normalizedHeaderTable = normalizeTableName(headerTableName,
tableType, headers,
+ CommonConstants.Controller.TABLE_NAME_HTTP_HEADER + " header");
+ String destinationTable = normalizedRequestTable != null ?
normalizedRequestTable
+ : normalizedHeaderTable;
+ String normalizedMetadataTable = null;
+ if (requireMatchingMetadataTable || destinationTable == null) {
+ normalizedMetadataTable =
+ normalizeTableName(metadataTableName, tableType, headers, "segment
metadata table name");
+ if (destinationTable == null) {
+ destinationTable = normalizedMetadataTable;
+ }
+ }
+ if (destinationTable == null) {
+ throw new ControllerApplicationException(LOGGER,
+ "Table name is required in the request, " +
CommonConstants.Controller.TABLE_NAME_HTTP_HEADER
+ + " header, or segment metadata",
+ Response.Status.BAD_REQUEST);
+ }
+
+ validateMatchingTableName(destinationTable, normalizedHeaderTable,
+ CommonConstants.Controller.TABLE_NAME_HTTP_HEADER + " header");
Review Comment:
Documented this in the new Compatibility section: when `Pinot-TableName` is
supplied, it must match the canonical destination and a mismatch returns 400.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskGenerator.java:
##########
@@ -145,7 +145,7 @@ public List<PinotTaskConfig>
generateTasks(List<TableConfig> tableConfigs) {
}
configs.put(MinionConstants.DOWNLOAD_URL_KEY,
segmentZKMetadata.getDownloadUrl());
configs.put(MinionConstants.UPLOAD_URL_KEY,
- _clusterInfoAccessor.getVipUrlForLeadController(tableName) +
"/segments");
+ _clusterInfoAccessor.getVipUrlForLeadController(tableName) +
"/v2/segments");
Review Comment:
Added load-bearing comments at both the Purge and Refresh v2 URL sites.
Those tasks can reuse the original artifact and source-table metadata; other
in-tree conversion generators remain on v1 only because they regenerate
destination-bound metadata, and any future artifact-reuse path must use v2.
--
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]
