Skip to content

[Native] Trim native prefix for system session property#21023

Merged
NikhilCollooru merged 1 commit intoprestodb:masterfrom
kewang1024:trim-native-prefix
Oct 4, 2023
Merged

[Native] Trim native prefix for system session property#21023
NikhilCollooru merged 1 commit intoprestodb:masterfrom
kewang1024:trim-native-prefix

Conversation

@kewang1024
Copy link
Collaborator

@kewang1024 kewang1024 commented Oct 3, 2023

Also add writer_native_spill_enabled session property.

@kewang1024 kewang1024 requested review from a team as code owners October 3, 2023 08:07
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.

@kewang1024 thanks for the change!

@kewang1024 kewang1024 force-pushed the trim-native-prefix branch 3 times, most recently from 7877ec1 to b97575e Compare October 3, 2023 22:41
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.

@kewang1024 LGTM. Can you check with @amitkdutta as we might need to update our internal job setup with the changed session properties. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Will it work with old coordinator where native prefix is not passed? We can keep the worker backward compatible so that it can work with native and without native prefixed properties. Once coordinator is updated, we can remove anything that does not have native prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will work if old coordinator doesn't pass native prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Then lets just enhance the unit test to keep the legacy case. Once coordinator is updated, we can remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it will work with old coordinator

@amitkdutta
Copy link
Contributor

@kewang1024 Lets keep the worker compatible with old coordinator.

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.

Thanks @kewang1024 for adding the tests.

@NikhilCollooru NikhilCollooru merged commit accd109 into prestodb:master Oct 4, 2023
@wanglinsong wanglinsong mentioned this pull request Dec 8, 2023
26 tasks
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.

4 participants