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/08/18 23:12:11 UTC
[GitHub] [pinot] yaostartree opened a new pull request, #9245: [Feature
yaostartree opened a new pull request, #9245:
URL: https://github.com/apache/pinot/pull/9245
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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 a diff in pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9245:
URL: https://github.com/apache/pinot/pull/9245#discussion_r949679994
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -172,7 +172,7 @@ public void init(SegmentGenerationJobSpec spec) {
public void run()
throws Exception {
// Get list of files to process.
- String[] files = _inputDirFS.listFiles(_inputDirURI, true);
+ String[] files = _inputDirFS.listFilesWithPattern(_inputDirURI, _spec.getIncludeFileNamePattern());
Review Comment:
let's also refactor
https://github.com/apache/pinot/pull/9245/files#diff-7bec8ff885254fee67d4ad50c4f06a7894976700df3fd2ef42af6532fedfea26L181-L203
into this function.
the name `listFilesWithPattern` suggest it should already finish the glob searching
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -37,6 +37,7 @@ private PinotFSFactory() {
}
public static final String LOCAL_PINOT_FS_SCHEME = "file";
+ public static final String RECURSIVE_BLOB_PREFIX = "glob:**";
Review Comment:
recursive hint from glob pattern should do recursive search whenever there's a `**` pattern included, b/c `**` can occur in the middle such as `subFolder/foo/**/bar`
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java:
##########
@@ -144,6 +143,22 @@ boolean exists(URI fileUri)
long length(URI fileUri)
throws IOException;
+ /**
+ * Lists all the files and directories at the location provided.
+ * Lists recursively if {@code inputFilePattern} starts with "glob:**".
+ * Throws IOException if this abstract pathname is not valid, or if an I/O error occurs.
+ * @param fileUri location of file
+ * @return an array of strings that contains file paths
+ * @throws IOException on IO failure. See specific implementation
+ */
+ default String[] listFilesWithPattern(URI fileUri, String inputFilePattern)
Review Comment:
let's not add the default implementation in `PinotFS` since this is a very generic interface.
I would suggest let's add this as a static method in `SegmentGenerationJobSpec`
```
public static String[] listMatchedFiles(PinotFS pinotFs, URI fileUri, String includePattern, String excludePattern) {
// ...
}
```
--
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] yaostartree commented on pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
Posted by GitBox <gi...@apache.org>.
yaostartree commented on PR #9245:
URL: https://github.com/apache/pinot/pull/9245#issuecomment-1221470816
As discussed, the new implementation also adds the behavior that when input file pattern is null, we also do recursive search in root dir since there are unit test that covers this so I think it should be current default behavior. We may need more discussion on how we want to handle 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] yaostartree commented on pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
Posted by GitBox <gi...@apache.org>.
yaostartree commented on PR #9245:
URL: https://github.com/apache/pinot/pull/9245#issuecomment-1221699281
After thinking about it again, I feel getting the recursive option from the input file pattern is safer and cleaner.
It doesn't break existing usage and it performs better. The code is also cleaner.
Adding an searchRecursively option and setting it to false by default is dangerous because it is different from existing behavior.
--
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] yaostartree commented on a diff in pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
Posted by GitBox <gi...@apache.org>.
yaostartree commented on code in PR #9245:
URL: https://github.com/apache/pinot/pull/9245#discussion_r950788194
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -172,7 +172,7 @@ public void init(SegmentGenerationJobSpec spec) {
public void run()
throws Exception {
// Get list of files to process.
- String[] files = _inputDirFS.listFiles(_inputDirURI, true);
+ String[] files = _inputDirFS.listFilesWithPattern(_inputDirURI, _spec.getIncludeFileNamePattern());
Review Comment:
Moved the listFilesWithPattern function to SegmentGenerationUtils.
--
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] yaostartree commented on a diff in pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
Posted by GitBox <gi...@apache.org>.
yaostartree commented on code in PR #9245:
URL: https://github.com/apache/pinot/pull/9245#discussion_r950788258
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -37,6 +37,7 @@ private PinotFSFactory() {
}
public static final String LOCAL_PINOT_FS_SCHEME = "file";
+ public static final String RECURSIVE_BLOB_PREFIX = "glob:**";
Review Comment:
Updated the code and added a test.
--
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] mcvsubbu commented on a diff in pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on code in PR #9245:
URL: https://github.com/apache/pinot/pull/9245#discussion_r949709634
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -172,7 +172,7 @@ public void init(SegmentGenerationJobSpec spec) {
public void run()
throws Exception {
// Get list of files to process.
- String[] files = _inputDirFS.listFiles(_inputDirURI, true);
+ String[] files = _inputDirFS.listFilesWithPattern(_inputDirURI, _spec.getIncludeFileNamePattern());
Review Comment:
What guards do we have in place to make sure that the spec does not do a crazy get-all kind of call?
--
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 a diff in pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9245:
URL: https://github.com/apache/pinot/pull/9245#discussion_r950261062
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -172,7 +172,7 @@ public void init(SegmentGenerationJobSpec spec) {
public void run()
throws Exception {
// Get list of files to process.
- String[] files = _inputDirFS.listFiles(_inputDirURI, true);
+ String[] files = _inputDirFS.listFilesWithPattern(_inputDirURI, _spec.getIncludeFileNamePattern());
Review Comment:
sadly we don't have any. the behavior is even worse in our current implementation: see: https://github.com/apache/pinot/issues/9016
--
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] 61yao closed pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
Posted by GitBox <gi...@apache.org>.
61yao closed pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
URL: https://github.com/apache/pinot/pull/9245
--
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] mcvsubbu commented on a diff in pull request #9245: [Feature] List files from inputDir with recursive option based on inputFilePatterns in IngestionJobLauncher
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on code in PR #9245:
URL: https://github.com/apache/pinot/pull/9245#discussion_r949709164
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java:
##########
@@ -144,6 +143,22 @@ boolean exists(URI fileUri)
long length(URI fileUri)
throws IOException;
+ /**
+ * Lists all the files and directories at the location provided.
+ * Lists recursively if {@code inputFilePattern} starts with "glob:**".
+ * Throws IOException if this abstract pathname is not valid, or if an I/O error occurs.
+ * @param fileUri location of file
+ * @return an array of strings that contains file paths
+ * @throws IOException on IO failure. See specific implementation
+ */
+ default String[] listFilesWithPattern(URI fileUri, String inputFilePattern)
Review Comment:
+1, the default implementation should be to throw an exception asking if you have permission from your mom doing this :) We have already fielded one security issue in this regard.
Also, the API comments should clearly indicate that this is a potential security hole, and its usage is unsafe.
--
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