yashmayya commented on code in PR #19231:
URL: https://github.com/apache/pinot/pull/19231#discussion_r3857133859
##########
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:
This drops promotion from v1: a segment built for table A can no longer be
uploaded to table B through `/segments`. The old doc advertised that capability
explicitly.
Re-authorizing against the resolved destination already closes the gap on
its own, since a v1 promotion would be authorized against B. So the
metadata-match requirement is extra hardening that costs compatibility. Is it
needed, or can v1 keep promotion now that the destination is authorized? Either
way this belongs in the description as a breaking change: external v1 callers
that promote will start getting 400.
##########
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:
This makes the `Pinot-TableName` header authoritative. It was read and
discarded before, and nothing in this repo sets it — only the new test — so the
new requirement has no coverage against a real client.
An external uploader that sets it to, say, the source table of a conversion
now gets a 400 where it previously succeeded. Worth a line in the description.
##########
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:
MergeRollup, UpsertCompaction, UpsertCompactMerge and RealtimeToOffline
still post to `/segments`. They are safe only because their executors always
regenerate the segment, so `segment.table.name` matches the destination. Purge
and Refresh are the two that can return the original `indexDir`.
That invariant is implicit and now load-bearing. If someone later adds a
reuse path to one of those executors it breaks at runtime, not at compile time.
Either move all the generators to `/v2/segments`, or record the invariant in a
comment here.
##########
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:
The request and header table-name checks now run here, after the segment has
been downloaded, decrypted and untarred. The type-mismatch check used to sit at
the top of `uploadSegment`, before any of that.
So `?tableName=myTable_REALTIME&tableType=OFFLINE` now pulls the whole
segment before returning 400, and for URI push that is a full remote fetch.
Neither the request name nor the header name depends on the segment, so both
could be normalized up front, leaving only the metadata comparison here.
--
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]
