Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Add an option to search input files recursively in ingestion job. The default is set to true to be backward compatible. #9265

Merged
merged 6 commits into from
Aug 24, 2022

Conversation

61yao
Copy link
Contributor

@61yao 61yao commented Aug 22, 2022

searchRecursively option is added in jobSpec. It is added to prevent the non-necessary recursive search for input files.
Currently, the option is set to true by default since that is existing behavior.
In next PR, we will change the default to be false after clients are aware of this change.

@61yao
Copy link
Contributor Author

61yao commented Aug 22, 2022

I don't think it is safe to add this flag and set the default option to false since current behavior is searching recursively by default. It is dangerous to change default behavior since we don't know what we will break.

@61yao
Copy link
Contributor Author

61yao commented Aug 22, 2022

Updated the default searchRecursive option to be true.

Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the contribution @yaostartree , this looks great overall. I have some minor comments. please kindly take a look

Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@61yao 61yao changed the title [Feature] Add an option to search input files recursively in ingestion job. The default is set to false. [Feature] Add an option to search input files recursively in ingestion job. The default is set to true to be backward compatible. Aug 23, 2022
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job extracting the common code into the util

Is the pinot-plugins.tar.gz file required for this PR? I feel it is mistakenly included

@61yao 61yao force-pushed the recursive_flag branch 2 times, most recently from a5082ad to da7aad0 Compare August 24, 2022 00:54
@61yao
Copy link
Contributor Author

61yao commented Aug 24, 2022

Yeah. Removed the pinot-plugins.tar.gz

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great job!

@Jackie-Jiang Jackie-Jiang merged commit ae87819 into apache:master Aug 24, 2022
@Jackie-Jiang Jackie-Jiang added the release-notes Referenced by PRs that need attention when compiling the next release notes label Aug 24, 2022
@61yao 61yao deleted the recursive_flag branch September 8, 2022 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement release-notes Referenced by PRs that need attention when compiling the next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants