Jackie-Jiang commented on code in PR #14093:
URL: https://github.com/apache/pinot/pull/14093#discussion_r1777862631
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -79,10 +78,11 @@ public class StreamConfig {
// Allow overriding it to use different offset criteria
private OffsetCriteria _offsetCriteria;
- // Indicates if the segment should be uploaded to the deep store's file
system or to the controller during the
- // segment commit protocol. By default, segment is uploaded to the
controller during commit.
- // If this flag is set to true, the segment is uploaded to deep store.
- private final boolean _serverUploadToDeepStore;
+ // Indicate StreamConfig flag for table if segment should be uploaded to the
deep store's file system or to the
+ // controller during the segment commit protocol. if config is not present
in Table StreamConfig
+ // _serverUploadToDeepStore is null and method isServerUploadToDeepStore()
overrides the default value with Server
+ // level config
+ private final String _serverUploadToDeepStore;
Review Comment:
Let's make this an object `Boolean` to different explicit `false` and not set
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -214,8 +212,9 @@ public static void validateConsumerType(String streamType,
Map<String, String> s
}
}
- public boolean isServerUploadToDeepStore() {
- return _serverUploadToDeepStore;
+ public boolean isServerUploadToDeepStore(boolean
defaultServerUploadToDeepStore) {
Review Comment:
Directly return the object `Boolean` without passing in the default. We may
annotate the return as `@Nullable`
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentCommitterFactory.java:
##########
@@ -54,7 +57,13 @@ public SegmentCommitterFactory(Logger segmentLogger,
ServerSegmentCompletionProt
public SegmentCommitter
createSegmentCommitter(SegmentCompletionProtocol.Request.Params params,
String controllerVipUrl)
throws URISyntaxException {
- boolean uploadToFs = _streamConfig.isServerUploadToDeepStore();
+ InstanceDataManagerConfig instanceDataManagerConfig =
_indexLoadingConfig.getInstanceDataManagerConfig();
+ PinotConfiguration config = instanceDataManagerConfig.getConfig();
Review Comment:
Add a method in `InstanceDataManagerConfig` to return whether to upload
segment to deep store. You should be able to see other examples in the class
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1071,6 +1071,9 @@ public static class Offline {
*/
public static final String SEGMENT_UPLOAD_START_TIME =
"segment.upload.start.time";
+ public static final String CONFIG_SEGMENT_SERVER_UPLOAD_TO_DEEP_STORE =
"segment.server.upload.to.deep.store";
Review Comment:
We don't usually configure this constant here. See
`HelixInstanceDataManagerConfig` for example
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
