mcvsubbu commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r445668344
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -194,38 +196,32 @@ private SuccessResponse uploadSegment(@Nullable String
tableName, FormDataMultiP
ControllerFilePathProvider provider =
ControllerFilePathProvider.getInstance();
String tempFileName = TMP_DIR_PREFIX + System.nanoTime();
tempDecryptedFile = new File(provider.getFileUploadTempDir(),
tempFileName);
+ tempEncryptedFile = new File(provider.getFileUploadTempDir(),
tempFileName + ENCRYPTED_SUFFIX);
tempSegmentDir = new File(provider.getUntarredFileTempDir(),
tempFileName);
- // Set default crypter to the noop crypter when no crypter header is sent
- // In this case, the noop crypter will not do any operations, so the
encrypted and decrypted file will have the same
- // file path.
- if (crypterClassName == null) {
- crypterClassName = NoOpPinotCrypter.class.getSimpleName();
- tempEncryptedFile = new File(provider.getFileUploadTempDir(),
tempFileName);
- } else {
- tempEncryptedFile = new File(provider.getFileUploadTempDir(),
tempFileName + ENCRYPTED_SUFFIX);
- }
-
- // TODO: Change when metadata upload added
- String metadataProviderClass = DefaultMetadataExtractor.class.getName();
+ boolean uploadedSegmentIsEncrypted =
!Strings.isNullOrEmpty(crypterClassNameInHeader);
- SegmentMetadata segmentMetadata;
+ File dstFile = uploadedSegmentIsEncrypted ? tempEncryptedFile :
tempDecryptedFile;
FileUploadDownloadClient.FileUploadType uploadType =
getUploadType(uploadTypeStr);
switch (uploadType) {
case URI:
- segmentMetadata =
- getMetadataForURI(crypterClassName, downloadUri,
tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
- metadataProviderClass);
+ getSegmentFileFromURI(downloadUri, dstFile);
break;
case SEGMENT:
- getFileFromMultipart(multiPart, tempEncryptedFile);
- segmentMetadata = getSegmentMetadata(crypterClassName,
tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
- metadataProviderClass);
+ getSegmentFileFromMultipart(multiPart, dstFile);
break;
default:
throw new UnsupportedOperationException("Unsupported upload type: "
+ uploadType);
}
+ if (uploadedSegmentIsEncrypted) {
+ decryptFile(crypterClassNameInHeader, tempEncryptedFile,
tempDecryptedFile);
+ }
+
+ // TODO: Change when metadata upload added
Review comment:
Can you create an issue for this and reference the issue number? It is
not clear what needs to be done here. I am guessing that the change is that if
metadata is added, then we don't need to extract metadata?
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/TableCache.java
##########
@@ -80,6 +80,14 @@ public String getActualColumnName(String tableName, String
columnName) {
return columnName;
}
+ public TableConfig getTableConfig(String tableName) {
+ return
_tableConfigChangeListener._tableConfigMap.get(canonicalize(tableName));
+ }
+
+ private String canonicalize(String name) {
Review comment:
Nit: Are you planning to add additional canonicalization logic? If not,
we are substituting one method for another, right?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +300,28 @@ private String extractHttpHeader(HttpHeaders headers,
String name) {
return value;
}
+ private Pair<Boolean, String> encryptSegmentIfNeeded(String
offlineTableName, File tempDecryptedFile, File tempEncryptedFile,
+ boolean uploadedSegmentIsEncrypted) {
+
+ // get crypter class name from table config
+ String crypterClassName =
_pinotHelixResourceManager.getCrypterClassNameFromTableConfig(offlineTableName);
+
Review comment:
How about returning right here if segment does not need encryption? The
logic is easier to follow then.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +300,28 @@ private String extractHttpHeader(HttpHeaders headers,
String name) {
return value;
}
+ private Pair<Boolean, String> encryptSegmentIfNeeded(String
offlineTableName, File tempDecryptedFile, File tempEncryptedFile,
+ boolean uploadedSegmentIsEncrypted) {
+
+ // get crypter class name from table config
+ String crypterClassName =
_pinotHelixResourceManager.getCrypterClassNameFromTableConfig(offlineTableName);
+
+ boolean segmentNeedsEncryption = !Strings.isNullOrEmpty(crypterClassName);
+ if (segmentNeedsEncryption && uploadedSegmentIsEncrypted) {
+ throw new ControllerApplicationException(LOGGER, String.format(
Review comment:
why is this an exception? Should this just not be a case where
encryption is not needed? Why are we explicitly preventing the client from
encrypting the segment over the wire?
I would suggest the following:
(1) Check the crypter class name. If they are the same in the header and
tableconfig, then accept the segment.
(2) If they are not same, we have a few choices: Accept the segment after
bumping a metric (and a warn log), or reject the segment with an exception. The
problem with rejecting the segment is if the crypter classes are some extension
of one another, and actually work fine, then we may be unnecessarily rejecting
it.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -303,46 +335,37 @@ private String getZkDownloadURIForSegmentUpload(String
rawTableName, String segm
}
}
- private SegmentMetadata getMetadataForURI(String crypterClassHeader, String
currentSegmentLocationURI,
- File tempEncryptedFile, File tempDecryptedFile, File tempSegmentDir,
String metadataProviderClass)
+ private void getSegmentFileFromURI(String currentSegmentLocationURI, File
destFile)
throws Exception {
- SegmentMetadata segmentMetadata;
if (currentSegmentLocationURI == null ||
currentSegmentLocationURI.isEmpty()) {
throw new ControllerApplicationException(LOGGER, "Failed to get
downloadURI, needed for URI upload",
Response.Status.BAD_REQUEST);
}
- LOGGER.info("Downloading segment from {} to {}",
currentSegmentLocationURI, tempEncryptedFile.getAbsolutePath());
- SegmentFetcherFactory.fetchSegmentToLocal(currentSegmentLocationURI,
tempEncryptedFile);
- segmentMetadata = getSegmentMetadata(crypterClassHeader,
tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
- metadataProviderClass);
- return segmentMetadata;
+ LOGGER.info("Downloading segment from {} to {}",
currentSegmentLocationURI, destFile.getAbsolutePath());
+ SegmentFetcherFactory.fetchSegmentToLocal(currentSegmentLocationURI,
destFile);
}
- private SegmentMetadata getSegmentMetadata(String crypterClassHeader, File
tempEncryptedFile, File tempDecryptedFile,
- File tempSegmentDir, String metadataProviderClass)
+ private SegmentMetadata getSegmentMetadata(File tempDecryptedFile, File
tempSegmentDir, String metadataProviderClass)
throws Exception {
-
- decryptFile(crypterClassHeader, tempEncryptedFile, tempDecryptedFile);
-
// Call metadata provider to extract metadata with file object uri
return
MetadataExtractorFactory.create(metadataProviderClass).extractMetadata(tempDecryptedFile,
tempSegmentDir);
}
- private void completeZkOperations(boolean enableParallelPushProtection,
HttpHeaders headers, File tempEncryptedFile,
+ private void completeZkOperations(boolean enableParallelPushProtection,
HttpHeaders headers, File uploadedSegmentFile,
String rawTableName, SegmentMetadata segmentMetadata, String
segmentName, String zkDownloadURI,
- boolean moveSegmentToFinalLocation)
+ boolean moveSegmentToFinalLocation, String crypter)
throws Exception {
URI finalSegmentLocationURI = URIUtils
.getUri(ControllerFilePathProvider.getInstance().getDataDirURI().toString(),
rawTableName,
URIUtils.encode(segmentName));
ZKOperator zkOperator = new ZKOperator(_pinotHelixResourceManager,
_controllerConf, _controllerMetrics);
- zkOperator.completeSegmentOperations(rawTableName, segmentMetadata,
finalSegmentLocationURI, tempEncryptedFile,
- enableParallelPushProtection, headers, zkDownloadURI,
moveSegmentToFinalLocation);
+ zkOperator.completeSegmentOperations(rawTableName, segmentMetadata,
finalSegmentLocationURI, uploadedSegmentFile,
+ enableParallelPushProtection, headers, zkDownloadURI,
moveSegmentToFinalLocation, crypter);
}
- private void decryptFile(String crypterClassHeader, File tempEncryptedFile,
File tempDecryptedFile) {
- PinotCrypter pinotCrypter = PinotCrypterFactory.create(crypterClassHeader);
- LOGGER.info("Using crypter class {}", pinotCrypter.getClass().getName());
+ private void decryptFile(String crypterClassName, File tempEncryptedFile,
File tempDecryptedFile) {
+ PinotCrypter pinotCrypter = PinotCrypterFactory.create(crypterClassName);
+ LOGGER.info("Using crypter class {} for decryption.",
pinotCrypter.getClass().getName());
Review comment:
please add the file names to this log, thanks
----------------------------------------------------------------
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
