-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Core: Support set system level properties with environmental variables #5659
Conversation
cc @rdblue @kbendick @szehon-ho, this is a small update, could you take a look when you are free? Thanks a lot. |
/** Whether to use the shared worker pool when planning table scans. */ | ||
public static final String SCAN_THREAD_POOL_ENABLED = "iceberg.scan.plan-in-worker-pool"; | ||
|
||
static boolean getBoolean(String systemProperty, boolean defaultValue) { | ||
public static final String SCAN_THREAD_POOL_ENABLED_ENV = "ICEBERG_SCAN_PLAN_IN_WORKER_POOL"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be explosed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used by core/src/main/java/org/apache/iceberg/util/ThreadPools.java
in a different package. I updated other envs to the default access level.
d46074a
to
0c6a8f1
Compare
@szehon-ho @rdblue can we take another look and merge if possible ? |
Hi guys, can you explain the motivation of the change? Not opposed, but reading the description but didn't quite understand it. To me, both seem to be more or less equivalent in ease-of-use (they are passed to the java process when it is spawned..) |
@szehon-ho In many production environments, there are some default parameter settings for spark driver/executor java options (eg: gc parameters). And those parameters are concated into one string. This makes it difficult to overwrite or modify those parameters to set iceberg system-level properties. While with ENV settings, we could set those system-level properties separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yea spark seems to support env variables to set certain things. Left some comments.
0c6a8f1
to
fe5a9d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. While the code is cleaner now, cc @rdblue if we are ok with new public interface here
Thanks @szehon-ho for the reviewing, and sorry got held up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, left one nit. Will wait a bit if there's any more comment, and merge if not
if (value != null) { | ||
try { | ||
return parseFunc.apply(value); | ||
} catch (NumberFormatException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: how about catch Exception , as we don't know what kind of Function we get?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated and add the log.
gentle ping @szehon-ho |
Merged, thanks @ConeyLiu ! |
Thanks @szehon-ho @zinking @rdblue |
This add supports setting system level properties with environmental variables. For example: we could control the number of threads for the default Iceberg thread pool with java system property:
iceberg.worker.num-threads
or env:ICEBERG_WORKER_NUM_THREADS
. This is indeed useful when we want to change those properties while the java system properties have some default settings and we do not want to change them.