You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/06/25 00:27:08 UTC
[GitHub] [incubator-pinot] sajjad-moradi opened a new pull request #5617: Add segment encryption on Controller based on table config
sajjad-moradi opened a new pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617
## Description
Currently if one wants to have their segments encrypted on Pinot cluster, they need to encrypt the segments before upload and then on the upload http request, `CRYPTER` header with crypter class name as the value needs to be set. In this PR, encryption is added on Controller side by introducing an optional config `crypterClassName` int table config. In UploadSegment path of Controller's REST API, this config is retrieved and if its value is not null, it triggers segment encryption using the specified crypter.
Note that to have less pressure on ZK for retrieving table config on each segment upload, an existing cache for table configs, `TableCache` in `HelixResourceManager` is being used.
## Testing Done
Ran Controller, Server, and Broker locally with the new changes and verified the desired behavior in different scenarios for uploading segments:
1. `CRYPTER` flag set to `NoOpPinotCrypter`
2. `crypterClassName` config set to `NoOpPinotCrypter`
3. Both 1 and 2
4. Neither
Also in each scenario, checked the value of crypter property of the uploaded segment ZK metadata.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r449215565
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -194,38 +196,31 @@ 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);
+ downloadSegmentFileFromURI(downloadUri, dstFile, tableName);
break;
case SEGMENT:
- getFileFromMultipart(multiPart, tempEncryptedFile);
- segmentMetadata = getSegmentMetadata(crypterClassName, tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
- metadataProviderClass);
+ createSegmentFileFromMultipart(multiPart, dstFile);
break;
default:
throw new UnsupportedOperationException("Unsupported upload type: " + uploadType);
}
+ if (uploadedSegmentIsEncrypted) {
Review comment:
If we do that, the reader needs to know the assumption that there's always a default noopcrypter which gets used when there's no encryption involved. I think using the proposed approach, it's more explicit and reads better - if the segment is encrypted, then decryption needs to happen.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448697821
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -177,13 +179,13 @@ public Response downloadSegment(
private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiPart multiPart,
boolean enableParallelPushProtection, HttpHeaders headers, Request request, boolean moveSegmentToFinalLocation) {
String uploadTypeStr = null;
- String crypterClassName = null;
+ String crypterClassNameInHeader = null;
Review comment:
it's better to pass the crypterName instead of the classname right? Its ok to have the ability to override it with a classname but it weird to have users pass classname in the request
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -194,38 +196,31 @@ 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);
+ downloadSegmentFileFromURI(downloadUri, dstFile, tableName);
Review comment:
why are we download the segment?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -251,6 +246,17 @@ private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiP
_controllerMetrics, _leadControllerManager.isLeaderForTable(offlineTableName))
.validateOfflineSegment(offlineTableName, segmentMetadata, tempSegmentDir);
+ // Encrypt segment
+ String crypterClassNameInTableConfig =
Review comment:
is this making a ZK call?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -194,38 +196,31 @@ 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);
+ downloadSegmentFileFromURI(downloadUri, dstFile, tableName);
break;
case SEGMENT:
- getFileFromMultipart(multiPart, tempEncryptedFile);
- segmentMetadata = getSegmentMetadata(crypterClassName, tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
- metadataProviderClass);
+ createSegmentFileFromMultipart(multiPart, dstFile);
break;
default:
throw new UnsupportedOperationException("Unsupported upload type: " + uploadType);
}
+ if (uploadedSegmentIsEncrypted) {
Review comment:
better to always go through the crypter with noopcrypter
##########
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:
IMO, its better to always go through the same path and just make the crypter no-op
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448506657
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/TableCache.java
##########
@@ -98,7 +102,7 @@ public synchronized void refresh() {
try {
TableConfig tableConfig = TableConfigUtils.fromZNRecord(znRecord);
String tableNameWithType = tableConfig.getTableName();
- _tableConfigMap.put(tableNameWithType, tableConfig);
+ _tableConfigMap.put(tableNameWithType.toLowerCase(), tableConfig);
Review comment:
Reverted. I was following the pattern with column and table name in this class.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r447344902
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -251,6 +247,20 @@ private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiP
_controllerMetrics, _leadControllerManager.isLeaderForTable(offlineTableName))
.validateOfflineSegment(offlineTableName, segmentMetadata, tempSegmentDir);
+ Pair<Boolean, String> encryptionInfo =
+ encryptSegmentIfNeeded(offlineTableName, tempDecryptedFile, tempEncryptedFile, uploadedSegmentIsEncrypted);
Review comment:
Done.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r447344931
##########
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)
Review comment:
Makes sense 👍 Refactored.
##########
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:
Done.
##########
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:
Done.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar edited a comment on pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
npawar edited a comment on pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#issuecomment-649730750
> @npawar should the new crypter class name go into ingestion config (because this is related to ingestion)? Or, as @Jackie-Jiang recommends, in the segment validation and retention?
I think SegmentSConfig is a better place than ingestionConfig. This is more like a segment property than a "how to ingest" property.
It definitely shouldn't be a top level property
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#issuecomment-652727702
> I would recommend putting this under the `SegmentsValidationAndRetentionConfig` instead of creating another top level config.
Why is this the right place for this config. IMO, SegmentsValidationAndRetentionConfig should be broken into multiple sections.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r449217652
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -194,38 +196,31 @@ 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);
+ downloadSegmentFileFromURI(downloadUri, dstFile, tableName);
Review comment:
Downloading logic was there before. I just renamed the functions.
Basically if the download uri, instead of the segment data, is provided, controllers download the segment from the given uri.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#issuecomment-649160749
I would recommend putting this under the `SegmentsValidationAndRetentionConfig` instead of creating another top level config.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r449215565
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -194,38 +196,31 @@ 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);
+ downloadSegmentFileFromURI(downloadUri, dstFile, tableName);
break;
case SEGMENT:
- getFileFromMultipart(multiPart, tempEncryptedFile);
- segmentMetadata = getSegmentMetadata(crypterClassName, tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
- metadataProviderClass);
+ createSegmentFileFromMultipart(multiPart, dstFile);
break;
default:
throw new UnsupportedOperationException("Unsupported upload type: " + uploadType);
}
+ if (uploadedSegmentIsEncrypted) {
Review comment:
If we do that, the reader needs to know the assumption that there's always a default noopcrypter which gets used when there's no encryption involved. IMO using the proposed approach, it's more explicit and reads better - if the segment is encrypted, then decryption needs to happen.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r447345710
##########
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:
I went with rejecting the segment option. If we encounter users having same crypter with different names, it'll be a small refactor to allow that change.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448007026
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +297,34 @@ private String extractHttpHeader(HttpHeaders headers, String name) {
return value;
}
+ private Boolean encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,
+ boolean uploadedSegmentIsEncrypted, String crypterUsedInUploadedSegment, String crypterClassNameInTableConfig) {
+
+ boolean segmentNeedsEncryption = !Strings.isNullOrEmpty(crypterClassNameInTableConfig);
+ if (!segmentNeedsEncryption) {
+ // do nothing
+ return false;
+ }
+
+ if (uploadedSegmentIsEncrypted) {
+ if (!crypterClassNameInTableConfig.equalsIgnoreCase(crypterUsedInUploadedSegment)) {
Review comment:
why?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448505771
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -303,46 +342,38 @@ private String getZkDownloadURIForSegmentUpload(String rawTableName, String segm
}
}
- private SegmentMetadata getMetadataForURI(String crypterClassHeader, String currentSegmentLocationURI,
- File tempEncryptedFile, File tempDecryptedFile, File tempSegmentDir, String metadataProviderClass)
+ private void downloadSegmentFileFromURI(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());
Review comment:
I think the same way, but I added the table name to the logs anyways. For the segment, it is not available here. Segment needs to be downloaded first and then its name can be retrieved from the segment metadata.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#issuecomment-653231550
> is Crypter part of the pinot-spi?
It is part of pinot-spi.
> Should we treat this similar to other plugins?
I'm not sure. I'll let more experienced ppl to chime in.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#issuecomment-651453842
> > I would recommend putting this under the `SegmentsValidationAndRetentionConfig` instead of creating another top-level config.
>
> can we rename 'SegmentsValidationAndRetentionConfig' to segmentConfig?
I tried to do that, but it added a lot of noise to the PR, so I decided to leave the renaming to another PR. I'll create a separate PR for it after this one goes in.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448527618
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +296,39 @@ private String extractHttpHeader(HttpHeaders headers, String name) {
return value;
}
+ Pair<String, File> encryptSegmentIfNeeded(File tempDecryptedFile, File tempEncryptedFile,
+ boolean isUploadedSegmentEncrypted, String crypterUsedInUploadedSegment, String crypterClassNameInTableConfig,
+ String segmentName, String tableName) {
+
+ boolean segmentNeedsEncryption = !Strings.isNullOrEmpty(crypterClassNameInTableConfig);
+
+ // form the output
+ File finalSegmentFile =
+ (isUploadedSegmentEncrypted || segmentNeedsEncryption) ? tempEncryptedFile : tempDecryptedFile;
+ String crypterClassName = Strings.isNullOrEmpty(crypterClassNameInTableConfig) ? crypterUsedInUploadedSegment
+ : crypterClassNameInTableConfig;
+ ImmutablePair<String, File> out = ImmutablePair.of(crypterClassName, finalSegmentFile);
+
+ if (!segmentNeedsEncryption) {
+ return out;
+ }
+
+ if (isUploadedSegmentEncrypted && !crypterClassNameInTableConfig.equals(crypterUsedInUploadedSegment)) {
+ throw new ControllerApplicationException(LOGGER, String.format(
+ "Uploaded segment is encrypted with '%s' while table config requires '%s' as crypter "
+ + "(segment name = '%s', table name = '%s').", crypterUsedInUploadedSegment,
+ crypterClassNameInTableConfig, segmentName, tableName), Response.Status.INTERNAL_SERVER_ERROR);
+ }
+
+ // encrypt segment
+ PinotCrypter pinotCrypter = PinotCrypterFactory.create(crypterClassNameInTableConfig);
+ LOGGER.info("Using crypter class '{}' for encrypting '{}' to '{}' (segment name = '{}}', table name = '{}').",
Review comment:
```suggestion
LOGGER.info("Using crypter class '{}' for encrypting '{}' to '{}' (segment name = '{}', table name = '{}').",
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r449276912
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -177,13 +179,13 @@ public Response downloadSegment(
private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiPart multiPart,
boolean enableParallelPushProtection, HttpHeaders headers, Request request, boolean moveSegmentToFinalLocation) {
String uploadTypeStr = null;
- String crypterClassName = null;
+ String crypterClassNameInHeader = null;
Review comment:
If you're talking about the difference between fully qualified name vs just the name of the crypter, then how can we differentiate two crypters which have the same name but different packages?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r447245653
##########
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:
Using canonicalize explicitly states the reason why we're making the those strings lower case which IMO makes the code more maintainable, but since you both think otherwise, I'm reverting it.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#issuecomment-649730750
> @npawar should the new crypter class name go into ingestion config (because this is related to ingestion)? Or, as @Jackie-Jiang recommends, in the segment validation and retention?
I think SegmentSConfig is a better place than ingestionConfig. This is more like a segment property than a "how to ingest" property.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448007226
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +297,34 @@ private String extractHttpHeader(HttpHeaders headers, String name) {
return value;
}
+ private Boolean encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,
Review comment:
Refactored.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448007026
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +297,34 @@ private String extractHttpHeader(HttpHeaders headers, String name) {
return value;
}
+ private Boolean encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,
+ boolean uploadedSegmentIsEncrypted, String crypterUsedInUploadedSegment, String crypterClassNameInTableConfig) {
+
+ boolean segmentNeedsEncryption = !Strings.isNullOrEmpty(crypterClassNameInTableConfig);
+ if (!segmentNeedsEncryption) {
+ // do nothing
+ return false;
+ }
+
+ if (uploadedSegmentIsEncrypted) {
+ if (!crypterClassNameInTableConfig.equalsIgnoreCase(crypterUsedInUploadedSegment)) {
Review comment:
Done.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r449213171
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -251,6 +246,17 @@ private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiP
_controllerMetrics, _leadControllerManager.isLeaderForTable(offlineTableName))
.validateOfflineSegment(offlineTableName, segmentMetadata, tempSegmentDir);
+ // Encrypt segment
+ String crypterClassNameInTableConfig =
Review comment:
No, it uses an existing TableCache which gets updated upon changes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#issuecomment-650222277
> I would recommend putting this under the `SegmentsValidationAndRetentionConfig` instead of creating another top-level config.
can we rename 'SegmentsValidationAndRetentionConfig' to segmentConfig?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu merged pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
mcvsubbu merged pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#issuecomment-652725239
is Crypter part of the pinot-spi? Should we treat this similar to other plugins?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r445723995
##########
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)
Review comment:
A `get` method returning void seems odd to me. Probably `s/get/download`?
##########
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:
If latter, probably not worth creating another layer of method call.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -251,6 +247,20 @@ private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiP
_controllerMetrics, _leadControllerManager.isLeaderForTable(offlineTableName))
.validateOfflineSegment(offlineTableName, segmentMetadata, tempSegmentDir);
+ Pair<Boolean, String> encryptionInfo =
+ encryptSegmentIfNeeded(offlineTableName, tempDecryptedFile, tempEncryptedFile, uploadedSegmentIsEncrypted);
Review comment:
crypterClassName should be an input to this method, not a return value? This method should just return the boolean.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r447345031
##########
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:
Makes sense. Refactored per your suggestion.
##########
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:
I'm not sure why TODO is added there. I looked at it git history and it's added on this PR: [3155](https://github.com/apache/incubator-pinot/pull/3153). If you (or other ppl) can confirm the intention there, i'll open up an issue for it.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448690902
##########
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:
FYI, we will need the support for metadata push pretty soon. But that is somewhat unrelated here, so just an FYI at this point in time.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448053367
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -303,46 +342,38 @@ private String getZkDownloadURIForSegmentUpload(String rawTableName, String segm
}
}
- private SegmentMetadata getMetadataForURI(String crypterClassHeader, String currentSegmentLocationURI,
- File tempEncryptedFile, File tempDecryptedFile, File tempSegmentDir, String metadataProviderClass)
+ private void downloadSegmentFileFromURI(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());
Review comment:
I am assuming that the URI tells us segment and table name here, otherwise, it is useful to add table/segment name i this log
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +296,39 @@ private String extractHttpHeader(HttpHeaders headers, String name) {
return value;
}
+ Pair<String, File> encryptSegmentIfNeeded(File tempDecryptedFile, File tempEncryptedFile,
+ boolean isUploadedSegmentEncrypted, String crypterUsedInUploadedSegment, String crypterClassNameInTableConfig,
+ String segmentName, String tableName) {
+
+ boolean segmentNeedsEncryption = !Strings.isNullOrEmpty(crypterClassNameInTableConfig);
+
+ // form the output
+ File finalSegmentFile =
+ (isUploadedSegmentEncrypted || segmentNeedsEncryption) ? tempEncryptedFile : tempDecryptedFile;
+ String crypterClassName = Strings.isNullOrEmpty(crypterClassNameInTableConfig) ? crypterUsedInUploadedSegment
+ : crypterClassNameInTableConfig;
+ ImmutablePair<String, File> out = ImmutablePair.of(crypterClassName, finalSegmentFile);
+
+ if (!segmentNeedsEncryption) {
+ return out;
+ }
+
+ if (isUploadedSegmentEncrypted && !crypterClassNameInTableConfig.equals(crypterUsedInUploadedSegment)) {
+ throw new ControllerApplicationException(LOGGER, String
+ .format("Uploaded segment is encrypted with '%s' while table config requires '%s' as crypter "
+ + "(segment name = '%s', table name = '%s').", crypterUsedInUploadedSegment,
+ crypterClassNameInTableConfig, segmentName, tableName), Response.Status.INTERNAL_SERVER_ERROR);
+ }
+
+ // encrypt segment
+ PinotCrypter pinotCrypter = PinotCrypterFactory.create(crypterClassNameInTableConfig);
+ LOGGER.info("Using crypter class {} for encrypting {} to {}.", crypterClassNameInTableConfig, tempDecryptedFile,
Review comment:
please add table name and segment name here. In production, we will see multiple parallel pushes going on, and it is useful to determine which segment/table we are talking about.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/TableCache.java
##########
@@ -98,7 +102,7 @@ public synchronized void refresh() {
try {
TableConfig tableConfig = TableConfigUtils.fromZNRecord(znRecord);
String tableNameWithType = tableConfig.getTableName();
- _tableConfigMap.put(tableNameWithType, tableConfig);
+ _tableConfigMap.put(tableNameWithType.toLowerCase(), tableConfig);
Review comment:
Not sure why we lower case here. Can you elaborate?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #5617: Add segment encryption on Controller based on table config
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#issuecomment-653168392
> > I would recommend putting this under the `SegmentsValidationAndRetentionConfig` instead of creating another top level config.
>
> Why is this the right place for this config. IMO, SegmentsValidationAndRetentionConfig should be broken into multiple sections.
That's why I added a TODO in this config: `TODO: Consider break this config into multiple configs`
At least we can rename it to `SegmentsConfig`.
But I don't think this is in the scope of this PR.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org