-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[native] Allow options for wget and tar commands during setup #24455
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
Conversation
8a9b0ac to
317a0ec
Compare
|
@czentgr any feedback? |
majetideepak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anandamideShakyan Can you fix the commit title as the PR title? Thanks.
317a0ec to
6e978a3
Compare
|
@anandamideShakyan the commit title must be |
6e978a3 to
31111dd
Compare
|
@anandamideShakyan My bad. Please include the [native] prefix as well :). |
|
Can you also rebase with the master branch? Thanks. |
31111dd to
33d2a31
Compare
majetideepak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! @anandamideShakyan
PingLiuPing
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @anandamideShakyan
Looking at the your fix and your PR description as following
- Avoid printing the files extracted by tar.
- Silence the download progress from curl by using an appropriate curl option.
- Suppress unnecessary CMake messages, logging only warnings instead of info or status messages.
- Introduce a new environment variable to enable the extensive logging when needed.
- These changes will help minimize log size while maintaining flexibility for detailed logging when required.
I cannot understand why adding -v to tar and a empty option to wget can fix the issues you described.
Can you please help to explain more?
Especially when adding -v option to tar command the output will include verbose information, and this is completely contrary to your original intention.
Thanks
|
Hi @PingLiuPing. To keep the behaviour consistent with the old setup I am passing empty option to wget and -v to tar. The users can set this to "-q" and "" explicitly when needed. See Deepak's comments on the |
|
@anandamideShakyan Thank you. Since because these two variables are not been used in other places such as velox scritps. So, I cannot undertand what's the benefit we can get by adding this two variables to allowing users to control the wget and tar for gperf. And |
|
@PingLiuPing. You can find the velox PR here: https://github.com/facebookincubator/velox/pull/12201/files. The main reason for this change was to reduce log size in SPS pipeline in lakehouse. This was first raised as an internal PR but then opensource PR was also opened. The log size did get reduced to less than 16MB which was the primary requirement see : https://github.ibm.com/lakehouse/presto/issues/2606. As for |
|
@anandamideShakyan , after checking the code changes I don't find any places use |
|
I agree. We should remove |
|
I have removed TAR_OPTIONS and raised a PR : #24854 |
Thanks @anandamideShakyan . |
Description
The current setup scripts generate excessive output, the goal is to reduce the log output by implementing the following changes:
Motivation and Context
The current setup scripts generate excessive log output, which can overwhelm users and make it difficult to identify important information. This excessive logging increases log file sizes unnecessarily, which can be problematic for storage and debugging. By implementing changes to reduce log output, we aim to streamline the logging process, improve readability, and maintain flexibility for detailed logging when required.
Impact