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 2022/10/28 13:03:30 UTC

[GitHub] [pinot] KKcorps opened a new pull request, #9681: Close Filesystem on exit with Minion Tasks

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

   When Filesystems are created outside the factory, we should ensure that they are closed on exit. This doesn't happen a lot of times in minion tasks and leads to memory leaks such as this - 
   https://coder-shankar.medium.com/aws-sdk-memory-leak-63e23dcdc6f0
   
   


-- 
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

[GitHub] [pinot] klsince commented on a diff in pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9681:
URL: https://github.com/apache/pinot/pull/9681#discussion_r1008716012


##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/segmentgenerationandpush/SegmentGenerationAndPushTaskExecutor.java:
##########
@@ -177,12 +178,24 @@ private void pushSegment(String tableName, Map<String, String> taskConfigs, URI
       outputSegmentDirURI = URI.create(taskConfigs.get(BatchConfigProperties.OUTPUT_SEGMENT_DIR_URI));
     }
     PinotFS outputFileFS = SegmentGenerationAndPushTaskUtils.getOutputPinotFS(taskConfigs, outputSegmentDirURI);
+    boolean closeOutputFSOnExit = true;

Review Comment:
   It seems a bit error prone to me to use this flag to tell if the fs is from a cache of a factory, e.g. the getOutputPinotFS () can return LOCAL_PINOT_FS and it is closed (although it's a noop). 
   
   How about always return a new instance from getOutputPinotFS(), so it should be closed all the time after use. It incurs some overhead but less error prone, and it's for task generator/executor (batching processing) so the overhead can be tolerable. 
   
   



-- 
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

[GitHub] [pinot] KKcorps commented on a diff in pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9681:
URL: https://github.com/apache/pinot/pull/9681#discussion_r1012155629


##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -74,9 +84,21 @@ public static void init(PinotConfiguration fsFactoryConfig) {
   }
 
   public static PinotFS create(String scheme) {

Review Comment:
   Yes, this is the reason I didn't rename.



-- 
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

[GitHub] [pinot] jackjlli commented on a diff in pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #9681:
URL: https://github.com/apache/pinot/pull/9681#discussion_r1008312355


##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/segmentgenerationandpush/SegmentGenerationAndPushTaskUtils.java:
##########
@@ -50,7 +49,7 @@ static PinotFS getInputPinotFS(Map<String, String> taskConfigs, URI fileURI)
       return pinotFS;
     }
     // Fallback to use the PinotFS created by Minion Server configs
-    return PinotFSFactory.create(fileURIScheme);
+    return null;

Review Comment:
   Should we update the comment 1 line above?



-- 
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

[GitHub] [pinot] walterddr commented on pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #9681:
URL: https://github.com/apache/pinot/pull/9681#issuecomment-1299373327

   another thought since we are talking about how to map fsClassName to pinotFS. 
   
   should we also address the fact that users might have different `fsConfiguration` for the same `fsClassName`? for example 2 S3PinotFS (same scheme) but different configuration for access keys. i don't think the current PinotFSFactory handles this


-- 
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

[GitHub] [pinot] KKcorps commented on a diff in pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9681:
URL: https://github.com/apache/pinot/pull/9681#discussion_r1008420066


##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/segmentgenerationandpush/SegmentGenerationAndPushTaskUtils.java:
##########
@@ -50,7 +49,7 @@ static PinotFS getInputPinotFS(Map<String, String> taskConfigs, URI fileURI)
       return pinotFS;
     }
     // Fallback to use the PinotFS created by Minion Server configs
-    return PinotFSFactory.create(fileURIScheme);
+    return null;

Review Comment:
   Thanks for pointing out! Removed it now.



-- 
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

[GitHub] [pinot] KKcorps merged pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
KKcorps merged PR #9681:
URL: https://github.com/apache/pinot/pull/9681


-- 
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

[GitHub] [pinot] KKcorps commented on pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #9681:
URL: https://github.com/apache/pinot/pull/9681#issuecomment-1298544251

   I can do that. But then, the question is what should one do if no FS configs are mentioned in the task config. Currently we get the FS from cache which uses minion FS configs.


-- 
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

[GitHub] [pinot] KKcorps commented on pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #9681:
URL: https://github.com/apache/pinot/pull/9681#issuecomment-1299546146

   @walterddr that is out of the scope for this. In minion tasks, user provided FS configs take priority over cached FS.


-- 
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

[GitHub] [pinot] KKcorps commented on a diff in pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9681:
URL: https://github.com/apache/pinot/pull/9681#discussion_r1012497391


##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/NoClosePinotFS.java:
##########
@@ -0,0 +1,136 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.filesystem;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URI;
+import java.util.List;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public class NoClosePinotFS implements PinotFS {

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.

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

[GitHub] [pinot] KKcorps commented on a diff in pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9681:
URL: https://github.com/apache/pinot/pull/9681#discussion_r1012298208


##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -39,24 +39,34 @@ private PinotFSFactory() {
   public static final String LOCAL_PINOT_FS_SCHEME = "file";
   private static final Logger LOGGER = LoggerFactory.getLogger(PinotFSFactory.class);
   private static final String CLASS = "class";
-  private static final Map<String, PinotFS> PINOT_FS_MAP = new HashMap<String, PinotFS>() {
+  private static final Map<String, PinotFSInfo> PINOT_FS_MAP = new HashMap<String, PinotFSInfo>() {
     {
-      put(LOCAL_PINOT_FS_SCHEME, new LocalPinotFS());
+      put(LOCAL_PINOT_FS_SCHEME, new PinotFSInfo(null, null, new LocalPinotFS()));
     }
   };
 
   public static void register(String scheme, String fsClassName, PinotConfiguration fsConfiguration) {
     try {
-      LOGGER.info("Initializing PinotFS for scheme {}, classname {}", scheme, fsClassName);
-      PinotFS pinotFS = PluginManager.get().createInstance(fsClassName);
-      pinotFS.init(fsConfiguration);
-      PINOT_FS_MAP.put(scheme, pinotFS);

Review Comment:
   You're right. This approach seems cleaner and much-less error-prone for caller. Made the 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.

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

[GitHub] [pinot] Jackie-Jiang commented on pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9681:
URL: https://github.com/apache/pinot/pull/9681#issuecomment-1298980906

   > I can do that. But then, the question is what should one do if no FS configs are mentioned in the task config. Currently we get the FS from cache which uses minion FS configs.
   
   2 ways to solve it:
   1. In `PinotFSFactory`, cache the `fsClassName` and `fsConfiguration` without creating the `PinotFS` implementation. When `create` is called, initialize the `PinotFS`
   2. Make a `PinotFS` delegate and store the delegate class in the `PinotFSFactory` which won't close the underlying `PinotFS`


-- 
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

[GitHub] [pinot] codecov-commenter commented on pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9681:
URL: https://github.com/apache/pinot/pull/9681#issuecomment-1295030927

   # [Codecov](https://codecov.io/gh/apache/pinot/pull/9681?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#9681](https://codecov.io/gh/apache/pinot/pull/9681?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4e9c5d3) into [master](https://codecov.io/gh/apache/pinot/commit/2d78b358ef491da33167d3d905273c4b8c8483b3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d78b35) will **decrease** coverage by `54.21%`.
   > The diff coverage is `2.43%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #9681       +/-   ##
   =============================================
   - Coverage     69.88%   15.67%   -54.22%     
   + Complexity     4853      175     -4678     
   =============================================
     Files          1939     1893       -46     
     Lines        103793   101913     -1880     
     Branches      15738    15539      -199     
   =============================================
   - Hits          72540    15973    -56567     
   - Misses        26140    84741    +58601     
   + Partials       5113     1199     -3914     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `15.67% <2.43%> (+0.03%)` | :arrow_up: |
   
   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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9681?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...nandpush/SegmentGenerationAndPushTaskExecutor.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3Mvc2VnbWVudGdlbmVyYXRpb25hbmRwdXNoL1NlZ21lbnRHZW5lcmF0aW9uQW5kUHVzaFRhc2tFeGVjdXRvci5qYXZh) | `0.00% <0.00%> (-66.91%)` | :arrow_down: |
   | [...andpush/SegmentGenerationAndPushTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3Mvc2VnbWVudGdlbmVyYXRpb25hbmRwdXNoL1NlZ21lbnRHZW5lcmF0aW9uQW5kUHVzaFRhc2tHZW5lcmF0b3IuamF2YQ==) | `2.06% <0.00%> (-32.00%)` | :arrow_down: |
   | [...tionandpush/SegmentGenerationAndPushTaskUtils.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3Mvc2VnbWVudGdlbmVyYXRpb25hbmRwdXNoL1NlZ21lbnRHZW5lcmF0aW9uQW5kUHVzaFRhc2tVdGlscy5qYXZh) | `0.00% <0.00%> (-27.28%)` | :arrow_down: |
   | [.../org/apache/pinot/plugin/filesystem/S3PinotFS.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1maWxlLXN5c3RlbS9waW5vdC1zMy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcGx1Z2luL2ZpbGVzeXN0ZW0vUzNQaW5vdEZTLmphdmE=) | `41.89% <100.00%> (+0.19%)` | :arrow_up: |
   | [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...n/java/org/apache/pinot/core/data/table/Table.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1484 more](https://codecov.io/gh/apache/pinot/pull/9681/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
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

[GitHub] [pinot] klsince commented on a diff in pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9681:
URL: https://github.com/apache/pinot/pull/9681#discussion_r1012312480


##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/NoClosePinotFS.java:
##########
@@ -0,0 +1,136 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.filesystem;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URI;
+import java.util.List;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public class NoClosePinotFS implements PinotFS {

Review Comment:
   nit: maybe make this private to this pkg, as not sure if it'd be used outside.



-- 
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

[GitHub] [pinot] walterddr commented on pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #9681:
URL: https://github.com/apache/pinot/pull/9681#issuecomment-1300642162

   thanks for the clarification. originally i thought you will change the key of the cached map but seems like it only changes the content which seems fine


-- 
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

[GitHub] [pinot] klsince commented on a diff in pull request #9681: Close Filesystem on exit with Minion Tasks

Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9681:
URL: https://github.com/apache/pinot/pull/9681#discussion_r1012084995


##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -74,9 +84,21 @@ public static void init(PinotConfiguration fsFactoryConfig) {
   }
 
   public static PinotFS create(String scheme) {
-    PinotFS pinotFS = PINOT_FS_MAP.get(scheme);
-    Preconditions.checkState(pinotFS != null, "PinotFS for scheme: %s has not been initialized", scheme);
-    return pinotFS;
+    PinotFSInfo pinotFSInfo = PINOT_FS_MAP.get(scheme);
+    Preconditions.checkState(pinotFSInfo != null, "PinotFS for scheme: %s has not been initialized", scheme);
+    return pinotFSInfo.getPinotFS();
+  }
+
+  public static PinotFS createNewInstance(String scheme) {
+    PinotFSInfo pinotFSInfo = PINOT_FS_MAP.get(scheme);
+    Preconditions.checkState(pinotFSInfo != null, "PinotFS for scheme: %s has not been initialized", scheme);

Review Comment:
   perhaps make the error msg "FS scheme: %s has not been `registered`" to be more specific



##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -39,24 +39,34 @@ private PinotFSFactory() {
   public static final String LOCAL_PINOT_FS_SCHEME = "file";
   private static final Logger LOGGER = LoggerFactory.getLogger(PinotFSFactory.class);
   private static final String CLASS = "class";
-  private static final Map<String, PinotFS> PINOT_FS_MAP = new HashMap<String, PinotFS>() {
+  private static final Map<String, PinotFSInfo> PINOT_FS_MAP = new HashMap<String, PinotFSInfo>() {
     {
-      put(LOCAL_PINOT_FS_SCHEME, new LocalPinotFS());
+      put(LOCAL_PINOT_FS_SCHEME, new PinotFSInfo(null, null, new LocalPinotFS()));
     }
   };
 
   public static void register(String scheme, String fsClassName, PinotConfiguration fsConfiguration) {
     try {
-      LOGGER.info("Initializing PinotFS for scheme {}, classname {}", scheme, fsClassName);
-      PinotFS pinotFS = PluginManager.get().createInstance(fsClassName);
-      pinotFS.init(fsConfiguration);
-      PINOT_FS_MAP.put(scheme, pinotFS);

Review Comment:
   The current changes LGTM. 
   
   But looks like using a delegate pattern might save more changes (particular on the caller side),
   
   e.g. call `PINOT_FS_MAP.put(scheme, new NoClosePinotFS(pinotFS))` at L53 here, and add the NoClosePinotFS as a private class in this factory class.
   
   ```
   private static class NoClosePinotFS implement PinotFS {
   ...
   public NoClosePinotFS(PinotFS delegate) {
      _delegate = delegate;
   }
   // ... delegate all other methods to `_delegate` but leave close() as noop
   public void close() {...}
   }
   
   // for the close method of PinotFSFactory.java
   public void close() {
      for (PinotFS pinotFS : PINOT_FS_MAP.values()) {    
         ((NoClosePinotFS)pinotFS)._delegate.close();
      }
   }
   ```
   
   cc @Jackie-Jiang guess this is what you proposed as 2nd option?



##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/segmentgenerationandpush/SegmentGenerationAndPushTaskUtils.java:
##########
@@ -67,8 +66,7 @@ static PinotFS getOutputPinotFS(Map<String, String> taskConfigs, URI fileURI)
       pinotFS.init(fsProps);
       return pinotFS;

Review Comment:
   how about add a public method PinotFSFactory.createNewInstance(fileURIScheme, fsClass, fsProps) or make PinotFSFactory. createPinotFSInstance() public and call it here?



##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -74,9 +84,21 @@ public static void init(PinotConfiguration fsFactoryConfig) {
   }
 
   public static PinotFS create(String scheme) {

Review Comment:
   Looks like renaming this method touches so many places, then perhaps add a comment for this method that it gets an cached instance and caller shouldn't close the instance.



-- 
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