Skip to content

Conversation

duxiao1212
Copy link
Contributor

Description

Motivation and Context

Add session properties for scale writer query configs

scaled_writer_rebalance_max_memory_usage_ratio
scaled_writer_max_partitions_per_writer
scaled_writer_min_partition_processed_bytes_rebalance_threshold
scaled_writer_min_processed_bytes_rebalance_threshold

Impact

Low impact

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • 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 ==

@duxiao1212 duxiao1212 requested a review from a team as a code owner December 11, 2024 22:32
@kewang1024
Copy link
Collaborator

We also need to add them to coordinator
An example: #23741

Can we squash the commits to one?

@pdabre12
Copy link
Contributor

On the coordinator side, the native session properties have been moved to NativeWorkerSessionPropertyProvider, an example: #23891

@duxiao1212
Copy link
Contributor Author

duxiao1212 commented Dec 12, 2024

On the coordinator side, the native session properties have been moved to NativeWorkerSessionPropertyProvider, an example: #23891

Thanks, @pdabre12 . However, I noticed that NativeWorkerSessionPropertyProvider is marked as deprecated, and there doesn't seem to be any replacement suggestion. This is causing some confusion. Should we remove the deprecated annotation or do you have any recommendations?

@duxiao1212 duxiao1212 requested review from steveburnett, elharo and a team as code owners December 12, 2024 20:02
@pdabre12
Copy link
Contributor

On the coordinator side, the native session properties have been moved to NativeWorkerSessionPropertyProvider, an example: #23891

Thanks, @pdabre12 . However, I noticed that NativeWorkerSessionPropertyProvider is marked as deprecated, and there doesn't seem to be any replacement suggestion. This is causing some confusion. Should we remove the deprecated annotation or do you have any recommendations?

It is marked as deprecated, because we support the retrieval of the session properties through the sidecar, the PR: :#23045. Ideally, we want to use this functionality but this is a temporary class where native session properties were re-located to until the adoption of the sidecar.

zation99
zation99 previously approved these changes Dec 12, 2024
Copy link
Contributor

@zation99 zation99 left a comment

Choose a reason for hiding this comment

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

lgtm

@prestodb-ci
Copy link
Contributor

Saved that user @duxiao1212 is from Meta

@duxiao1212 duxiao1212 requested a review from xiaoxmeng December 13, 2024 19:01
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@duxiao1212 thanks for the change!

@duxiao1212 duxiao1212 closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:Meta PR from Meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants