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

Enhancement to respect user-defined log.path in launcher.py #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Akanksha-kedia
Copy link

@Akanksha-kedia Akanksha-kedia commented Mar 22, 2024

This PR addresses an enhancement in launcher.py to handle the log.path setting from node.properties. Currently, regardless of the value set for log.path in node.properties, the launcher.py script always uses the default value. This behavior is not correct as it does not respect the user's configuration.

Context:

This issue arises when launcher.py is run and the log.path is explicitly provided by the user in node.properties. The script does not respect this setting and continues to use the default value.

Motivation:

The motivation for this enhancement is to respect the user's explicit configuration of log.path in node.properties. The user should have the ability to control the configuration from their end. This change will make the script more adaptable to different user configurations and improve the user experience.

Understanding:

To implement this enhancement, the o.server_dir variable, which is used to construct the server log path, is set to the value of log.path from node.properties when it's explicitly provided by the user. This allows the user's setting to override the default value.

If log.path is not provided in node.properties, o.server_dir is set to a default path. This ensures that o.server_dir is always a valid path, even when log.path is not provided.

Details:

The issue arises when the log.path is explicitly set by the user in node.properties and the service is restarted. Despite the user's explicit configuration, the system prints pid -Dlog.path= instead of the user-defined value.
Screenshot 2024-02-28 at 10 08 01 AM
Screenshot 2024-02-28 at 10 08 29 AM

image

Test plan :
Screenshot 2024-03-22 at 12 10 57 PM
Screenshot 2024-03-22 at 12 11 41 PM

== RELEASE NOTES ==

General Changes
*Improve launcher.py to respect user-defined log.path.

@Akanksha-kedia
Copy link
Author

@tdcmeehan @steveburnett @imjalpreet please review.

@steveburnett
Copy link

Please add a release note entry if necessary, following the release notes guidelines.

@Akanksha-kedia
Copy link
Author

@steveburnett done

@Akanksha-kedia
Copy link
Author

@tdcmeehan please review.

@Akanksha-kedia
Copy link
Author

@tdcmeehan

@Akanksha-kedia
Copy link
Author

@tdcmeehan please review @imjalpreet @ajaygeorge

@Akanksha-kedia
Copy link
Author

@tdcmeehan @steveburnett @imjalpreet @ajaygeorge please review.

@steveburnett
Copy link

Please review the Order of Changes in the Presto Release Notes Guidelines, and revise your current release note entry appropriately.

@Akanksha-kedia
Copy link
Author

@tdcmeehan please review.

@wanglinsong wanglinsong requested a review from a team as a code owner August 1, 2024 08:03
@wanglinsong wanglinsong requested a review from presto-oss August 1, 2024 08:03
@steveburnett
Copy link

This is blocking prestodb/presto#22271.

@Akanksha-kedia
Copy link
Author

Akanksha-kedia commented Sep 13, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants