You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "raghavyadav01 (via GitHub)" <gi...@apache.org> on 2024/09/26 18:58:53 UTC

[PR] Add a server level config for segment server upload to deep store. [pinot]

raghavyadav01 opened a new pull request, #14093:
URL: https://github.com/apache/pinot/pull/14093

   Commit contains following changes.
   1. Add a cluster level flag to provide default value server upload to deep store setting for tables which does not have stream config property serverUploadToDeepStore.
   2. Validated the changes by running quick start locally. 


-- 
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 closed pull request #14093: Add a server level config for segment server upload to deep store.
URL: https://github.com/apache/pinot/pull/14093


-- 
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 commented on code in PR #14093:
URL: https://github.com/apache/pinot/pull/14093#discussion_r1779097365


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManagerConfig.java:
##########
@@ -135,6 +135,9 @@ public class HelixInstanceDataManagerConfig implements InstanceDataManagerConfig
   private static final String EXTERNAL_VIEW_DROPPED_MAX_WAIT_MS = "external.view.dropped.max.wait.ms";
   private static final String EXTERNAL_VIEW_DROPPED_CHECK_INTERVAL_MS = "external.view.dropped.check.interval.ms";
 
+  public static final String SEGMENT_SERVER_UPLOAD_TO_DEEP_STORE = "segment.server.upload.to.deep.store";

Review Comment:
   Fixed.



##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/SegmentCommitterFactoryTest.java:
##########
@@ -27,16 +27,17 @@
 import org.apache.pinot.common.protocols.SegmentCompletionProtocol;
 import org.apache.pinot.segment.local.segment.index.loader.IndexLoadingConfig;
 import org.apache.pinot.server.realtime.ServerSegmentCompletionProtocolHandler;
+import org.apache.pinot.spi.config.instance.InstanceDataManagerConfig;
 import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.stream.StreamConfigProperties;
 import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
 import org.mockito.Mockito;
 import org.slf4j.Logger;
 import org.testng.Assert;
 import org.testng.annotations.Test;
 
-

Review Comment:
   Fixed. 



-- 
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 commented on code in PR #14093:
URL: https://github.com/apache/pinot/pull/14093#discussion_r1779097594


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManagerConfig.java:
##########
@@ -330,4 +333,10 @@ public PinotConfiguration getAuthConfig() {
   public Map<String, Map<String, String>> getTierConfigs() {
     return _tierConfigs;
   }
+
+  @Override
+  public boolean getDefaultServerUploadToDeepStore() {

Review Comment:
   Fixed. 



-- 
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14093:
URL: https://github.com/apache/pinot/pull/14093#issuecomment-2377781181

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14093?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`55e071c`)](https://app.codecov.io/gh/apache/pinot/commit/55e071c9fe4d9c3fa117ee033111cda427a64854?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 1094 commits behind head on master.
   
   > :exclamation:  There is a different number of reports uploaded between BASE (59551e4) and HEAD (55e071c). Click for more details.
   > 
   > <details><summary>HEAD has 48 uploads less than BASE</summary>
   >
   >| Flag | BASE (59551e4) | HEAD (55e071c) |
   >|------|------|------|
   >|integration|7|2|
   >|integration2|3|2|
   >|temurin|12|2|
   >|java-21|7|2|
   >|skip-bytebuffers-true|3|1|
   >|skip-bytebuffers-false|7|1|
   >|unittests|5|0|
   >|unittests1|2|0|
   >|java-11|5|0|
   >|unittests2|3|0|
   >|integration1|2|0|
   >|custom-integration1|2|0|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #14093       +/-   ##
   =============================================
   - Coverage     61.75%    0.00%   -61.76%     
   =============================================
     Files          2436        3     -2433     
     Lines        133233        6   -133227     
     Branches      20636        0    -20636     
   =============================================
   - Hits          82274        0    -82274     
   + Misses        44911        6    -44905     
   + Partials       6048        0     -6048     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
   | [integration1](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration2](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
   | [java-11](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-21](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.63%)` | :arrow_down: |
   | [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.75%)` | :arrow_down: |
   | [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-27.73%)` | :arrow_down: |
   | [temurin](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.76%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14093?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   


-- 
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 commented on code in PR #14093:
URL: https://github.com/apache/pinot/pull/14093#discussion_r1777891331


##########
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:
   Fixed. 



##########
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:
   Fixed. 



-- 
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 closed pull request #14093: Add a server level config for segment server upload to deep store.
URL: https://github.com/apache/pinot/pull/14093


-- 
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 closed pull request #14093: Add a server level config for segment server upload to deep store.
URL: https://github.com/apache/pinot/pull/14093


-- 
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #14093:
URL: https://github.com/apache/pinot/pull/14093#discussion_r1779007894


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManagerConfig.java:
##########
@@ -135,6 +135,9 @@ public class HelixInstanceDataManagerConfig implements InstanceDataManagerConfig
   private static final String EXTERNAL_VIEW_DROPPED_MAX_WAIT_MS = "external.view.dropped.max.wait.ms";
   private static final String EXTERNAL_VIEW_DROPPED_CHECK_INTERVAL_MS = "external.view.dropped.check.interval.ms";
 
+  public static final String SEGMENT_SERVER_UPLOAD_TO_DEEP_STORE = "segment.server.upload.to.deep.store";

Review Comment:
   We should be able to simply the name because this config is under the context of server config (with prefix `pinot.server.instance`)
   ```suggestion
     public static final String UPLOAD_SEGMENT_TO_DEEP_STORE = "segment.upload.to.deep.store";
   ```



##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManagerConfig.java:
##########
@@ -330,4 +333,10 @@ public PinotConfiguration getAuthConfig() {
   public Map<String, Map<String, String>> getTierConfigs() {
     return _tierConfigs;
   }
+
+  @Override
+  public boolean getDefaultServerUploadToDeepStore() {

Review Comment:
   ```suggestion
     public boolean isUploadSegmentToDeepStore() {
   ```



##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/SegmentCommitterFactoryTest.java:
##########
@@ -27,16 +27,17 @@
 import org.apache.pinot.common.protocols.SegmentCompletionProtocol;
 import org.apache.pinot.segment.local.segment.index.loader.IndexLoadingConfig;
 import org.apache.pinot.server.realtime.ServerSegmentCompletionProtocolHandler;
+import org.apache.pinot.spi.config.instance.InstanceDataManagerConfig;
 import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.stream.StreamConfigProperties;
 import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
 import org.mockito.Mockito;
 import org.slf4j.Logger;
 import org.testng.Assert;
 import org.testng.annotations.Test;
 
-

Review Comment:
   (nit) Is the code format set up correctly? My IDE keeps 2 empty lines between `import` and `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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 commented on code in PR #14093:
URL: https://github.com/apache/pinot/pull/14093#discussion_r1779085297


##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/SegmentCommitterFactoryTest.java:
##########
@@ -27,16 +27,17 @@
 import org.apache.pinot.common.protocols.SegmentCompletionProtocol;
 import org.apache.pinot.segment.local.segment.index.loader.IndexLoadingConfig;
 import org.apache.pinot.server.realtime.ServerSegmentCompletionProtocolHandler;
+import org.apache.pinot.spi.config.instance.InstanceDataManagerConfig;
 import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.stream.StreamConfigProperties;
 import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
 import org.mockito.Mockito;
 import org.slf4j.Logger;
 import org.testng.Assert;
 import org.testng.annotations.Test;
 
-

Review Comment:
   I followed the steps here https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#intellij Is there anything else I need to do ? 



-- 
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-unsubscribe@pinot.apache.org

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


Re: [PR] Add a server level config for segment server upload to deep store. [pinot]

Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 commented on code in PR #14093:
URL: https://github.com/apache/pinot/pull/14093#discussion_r1777891167


##########
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:
   Fixed it. 



##########
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:
   Fixed. 



-- 
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-unsubscribe@pinot.apache.org

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