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

[native] Add support for optional velox.properties file #21962

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Feb 19, 2024

Description

Velox config has no required fields and therefore velox.properties should be optional.
From a Presto user perspective, existing config files (config.properties, node.properties)
for Java should work for Prestissimo as well.

Motivation and Context

Make Prestissimo config aligned to Presto Java

Impact

velox.properties is now optional.

Test Plan

Tested locally.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.
== NO RELEASE NOTE ==

@majetideepak majetideepak changed the title [native] Add support for optional velox.properties config [native] Add support for optional velox.properties file Feb 19, 2024
Copy link
Contributor

@amitkdutta amitkdutta left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @majetideepak

We had a discussion in #20406
My personal opinion is we stick with config properties and remove velox properties file - one less config to manage and eases deployment.

@majetideepak
Copy link
Collaborator Author

remove velox properties file

@amitkdutta Let's bring this up in the next Prestissimo WG meeting. CC: @aditi-pandit

@majetideepak majetideepak merged commit 2bd3f5e into prestodb:master Feb 20, 2024
59 checks passed
@amitkdutta
Copy link
Contributor

remove velox properties file

@amitkdutta Let's bring this up in the next Prestissimo WG meeting. CC: @aditi-pandit

CC: @spershin

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