Add a config property for excluding invalid worker session properties #23968

pdabre12 · 2024-11-06T23:15:07Z

Resolves: #23827

== RELEASE NOTES ==

General Changes
* Add configuration property ``exclude-invalid-worker-session-properties``. : pr:`23968`

steveburnett

Thanks for the doc! Nit for tense. Pull branch, local doc build, formatting looks good.

presto-docs/src/main/sphinx/admin/properties.rst

steveburnett · 2024-11-07T13:55:11Z

Thanks for the release note entry! A couple of nits in the formatting:

== RELEASE NOTES ==

General Changes
* Add configuration property ``exclude-java-worker-session-properties``. : pr:`23968`

pdabre12 · 2024-11-07T20:22:31Z

@steveburnett Thank you for the suggestions. Please have another look.

pdabre12 · 2024-11-07T20:27:36Z

CC: @tdcmeehan @rschlussel

steveburnett

LGTM! (docs)

Pull updated branch, reviewed the new local doc build, looks good. Thanks!

presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java

tdcmeehan · 2024-11-11T20:54:25Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

@@ -805,7 +805,9 @@ public ListeningExecutorService createResourceManagerExecutor(ResourceManagerCon
        // Worker session property providers
        MapBinder<String, WorkerSessionPropertyProvider> mapBinder =
                newMapBinder(binder, String.class, WorkerSessionPropertyProvider.class);
-        mapBinder.addBinding("java-worker").to(JavaWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON);
+        if (!featuresConfig.isExcludeJavaWorkerSessionProperties()) {


Suggested change

if (!featuresConfig.isExcludeJavaWorkerSessionProperties()) {

if (!serverConfig.isCoordinatorSidecarEnabled() || !featuresConfig.isExcludeJavaWorkerSessionProperties()) {

I think we should always include them for Java clusters.

rschlussel · 2024-11-11T20:57:50Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

-        mapBinder.addBinding("java-worker").to(JavaWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON);
+        if (!featuresConfig.isExcludeJavaWorkerSessionProperties()) {
+            mapBinder.addBinding("java-worker").to(JavaWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON);
+        }
        if (!serverConfig.isCoordinatorSidecarEnabled()) {
            mapBinder.addBinding("native-worker").to(NativeWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON);


Do we need the same for native-worker session properties? Maybe the config could have a generic name to exclude properties that aren't relevant for that worker type.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

pdabre12 · 2024-12-03T00:18:11Z

@tdcmeehan @rschlussel Made the changes, now setting the config property should help exclude properties that aren't relevant for that worker type.
Please have another look. Thanks !

rschlussel · 2024-12-03T20:59:13Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

-        mapBinder.addBinding("java-worker").to(JavaWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON);
-        if (!serverConfig.isCoordinatorSidecarEnabled()) {
-            mapBinder.addBinding("native-worker").to(NativeWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON);
+        if (featuresConfig.isExcludeInvalidWorkerSessionProperties()) {


i think it would be easier to understand if the logic were rearranged a bit like this:

if(featuresConfig.isNativeExecutionEnabled()) { if(!server.isCoordinatorSidecarEnabled()) { mapBinder.addBinding("native-worker").to(NativeWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON); } if(!featuresConfig.isExcludeInvalidWorkerSessionProperties()) { mapBinder.addBinding("java-worker").to(JavaWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON); } } else { mapBinder.addBinding("java-worker").to(JavaWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON); if(!featuresConfig.isExcludeInvalidWorkerSessionProperties()) { mapBinder.addBinding("native-worker").to(NativeWorkerSessionPropertyProvider.class).in(Scopes.SINGLETON); } }

Thanks for the suggestion, updated.

rschlussel · 2024-12-03T21:01:22Z

...src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPluginSystemPropertyProvider.java

@@ -72,14 +72,7 @@ public void testShowSession()
    @Test
    public void testSetJavaWorkerSessionProperty()


need a test that when exclude-invalid-session-properties is false that the session properties are visible. (and make sure we have true and false tests both for java and cpp.

Test cases added:

Native cluster invalid test :

presto/presto-native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPluginSystemPropertyProvider.java

Line 73 in e36b36e

public void testSetJavaWorkerSessionProperty()

Native cluster valid test:

presto/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java

Line 1186 in e36b36e

public void testSetSessionJavaWorkerSessionProperty()

True and false tests for java clusters.

pdabre12 · 2025-01-31T21:00:50Z

@rschlussel Can you please have another look? Thanks.

tdcmeehan · 2025-01-31T21:39:22Z

...java/com/facebook/presto/tests/TestSetWorkerSessionPropertiesExcludingInvalidProperties.java

+
+import static com.facebook.presto.testing.TestingSession.testSessionBuilder;
+
+public class TestSetWorkerSessionPropertiesExcludingInvalidProperties


Let's just remove the inheritance and create two classes. I feel like that's more straightforward than excluding tests depending on the subclass. Some duplication is OK here.

Updated, thanks.

pdabre12 force-pushed the add-java-worker-exclusion-config-property branch from 3ec1cc9 to a9f640f Compare November 6, 2024 23:19

steveburnett requested changes Nov 7, 2024

View reviewed changes

presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved

pdabre12 force-pushed the add-java-worker-exclusion-config-property branch from a9f640f to 1c80ce0 Compare November 7, 2024 20:22

pdabre12 marked this pull request as ready for review November 7, 2024 20:27

pdabre12 requested review from elharo, a team, jaystarshot, feilong-liu and ClarenceThreepwood as code owners November 7, 2024 20:27

pdabre12 requested a review from presto-oss November 7, 2024 20:27

steveburnett previously approved these changes Nov 7, 2024

View reviewed changes

tdcmeehan reviewed Nov 11, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java Outdated Show resolved Hide resolved

tdcmeehan self-assigned this Nov 11, 2024

pdabre12 dismissed steveburnett’s stale review via 4da5af8 November 11, 2024 18:36

pdabre12 force-pushed the add-java-worker-exclusion-config-property branch 2 times, most recently from 4da5af8 to a0ab00f Compare November 11, 2024 18:38

tdcmeehan requested changes Nov 11, 2024

View reviewed changes

rschlussel reviewed Nov 11, 2024

View reviewed changes

pdabre12 mentioned this pull request Nov 14, 2024

[native] Add TPC-DS connector #23067

Open

6 tasks

pdabre12 force-pushed the add-java-worker-exclusion-config-property branch from a0ab00f to 9b3f472 Compare December 3, 2024 00:15

pdabre12 requested a review from a team as a code owner December 3, 2024 00:15

pdabre12 changed the title ~~Add a config property for excluding java-worker session properties in a native cluster~~ Add a config property for excluding invalid worker session properties Dec 3, 2024

pdabre12 requested review from rschlussel and tdcmeehan December 3, 2024 00:18

rschlussel reviewed Dec 3, 2024

View reviewed changes

pdabre12 force-pushed the add-java-worker-exclusion-config-property branch from 9b3f472 to e36b36e Compare January 31, 2025 20:50

tdcmeehan reviewed Jan 31, 2025

View reviewed changes

Add a config property for excluding invalid worker session properties

f3033e0

pdabre12 force-pushed the add-java-worker-exclusion-config-property branch from e36b36e to f3033e0 Compare January 31, 2025 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a config property for excluding invalid worker session properties #23968

Add a config property for excluding invalid worker session properties #23968

pdabre12 commented Nov 6, 2024 •

edited

Loading

steveburnett left a comment

steveburnett commented Nov 7, 2024 •

edited

Loading

pdabre12 commented Nov 7, 2024

pdabre12 commented Nov 7, 2024

steveburnett left a comment

tdcmeehan Nov 11, 2024

rschlussel Nov 11, 2024

pdabre12 commented Dec 3, 2024

rschlussel Dec 3, 2024

pdabre12 Jan 31, 2025

rschlussel Dec 3, 2024

pdabre12 Jan 31, 2025 •

edited

Loading

pdabre12 commented Jan 31, 2025

tdcmeehan Jan 31, 2025

pdabre12 Jan 31, 2025

	if (!featuresConfig.isExcludeJavaWorkerSessionProperties()) {
	if (!serverConfig.isCoordinatorSidecarEnabled() \|\| !featuresConfig.isExcludeJavaWorkerSessionProperties()) {

		@@ -72,14 +72,7 @@ public void testShowSession()
		@Test
		public void testSetJavaWorkerSessionProperty()


		import static com.facebook.presto.testing.TestingSession.testSessionBuilder;

		public class TestSetWorkerSessionPropertiesExcludingInvalidProperties

Add a config property for excluding invalid worker session properties #23968

Are you sure you want to change the base?

Add a config property for excluding invalid worker session properties #23968

Conversation

pdabre12 commented Nov 6, 2024 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett commented Nov 7, 2024 • edited Loading

pdabre12 commented Nov 7, 2024

pdabre12 commented Nov 7, 2024

steveburnett left a comment

Choose a reason for hiding this comment

tdcmeehan Nov 11, 2024

Choose a reason for hiding this comment

rschlussel Nov 11, 2024

Choose a reason for hiding this comment

pdabre12 commented Dec 3, 2024

rschlussel Dec 3, 2024

Choose a reason for hiding this comment

pdabre12 Jan 31, 2025

Choose a reason for hiding this comment

rschlussel Dec 3, 2024

Choose a reason for hiding this comment

pdabre12 Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

pdabre12 commented Jan 31, 2025

tdcmeehan Jan 31, 2025

Choose a reason for hiding this comment

pdabre12 Jan 31, 2025

Choose a reason for hiding this comment

pdabre12 commented Nov 6, 2024 •

edited

Loading

steveburnett commented Nov 7, 2024 •

edited

Loading

pdabre12 Jan 31, 2025 •

edited

Loading