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

[Improvement] Replace ConfigEntry with ConfigOption in RssSparkConfig #1302

Open
2 of 3 tasks
zuston opened this issue Nov 9, 2023 · 4 comments
Open
2 of 3 tasks
Assignees
Labels
good first issue Good for newcomers

Comments

@zuston
Copy link
Member

zuston commented Nov 9, 2023

Code of Conduct

Search before asking

  • I have searched in the issues and found no similar issues.

What would you like to be improved?

We will use the ConfigOption to unify all client and server config style. But currently, there are some original code using the ConfigEntry, which is necessary to replace by the ConfigEntry

How should we improve?

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!
@zuston zuston added the good first issue Good for newcomers label Nov 9, 2023
@guixiaowen
Copy link
Contributor

@zuston i am new here, let me have a try

guixiaowen added a commit to guixiaowen/incubator-uniffle that referenced this issue Nov 14, 2023
guixiaowen added a commit to guixiaowen/incubator-uniffle that referenced this issue Nov 24, 2023
guixiaowen added a commit to guixiaowen/incubator-uniffle that referenced this issue Nov 24, 2023
guixiaowen added a commit to guixiaowen/incubator-uniffle that referenced this issue Nov 24, 2023
guixiaowen added a commit to guixiaowen/incubator-uniffle that referenced this issue Nov 24, 2023
guixiaowen added a commit to guixiaowen/incubator-uniffle that referenced this issue Nov 26, 2023
guixiaowen added a commit to guixiaowen/incubator-uniffle that referenced this issue Nov 26, 2023
guixiaowen added a commit to guixiaowen/incubator-uniffle that referenced this issue Dec 12, 2023
guixiaowen added a commit to guixiaowen/incubator-uniffle that referenced this issue Dec 12, 2023
@zhengchenyu
Copy link
Collaborator

Do we need to finalize the design about #1302 #1303 #1304 ?

RssSparkConfig/RssTezConfig/RssMRConfig or SparkConf/Configuration need to spark/tez/mr prefix?
And how about RssClientConfig? Do we need RssSparkConfig/RssTezConfig/RssMRConfig to be extend from RssClientConfig?

@zuston @jerqi @lifeSo Can you give us some suggesstion?

@lifeSo
Copy link
Collaborator

lifeSo commented Jan 11, 2024

Do we need to finalize the design about #1302 #1303 #1304 ?

RssSparkConfig/RssTezConfig/RssMRConfig or SparkConf/Configuration need to spark/tez/mr prefix? And how about RssClientConfig? Do we need RssSparkConfig/RssTezConfig/RssMRConfig to be extend from RssClientConfig?

@zuston @jerqi @lifeSo Can you give us some suggesstion?

I think at least TezClientConf need 'tez.' prefix, because there is some plase can not direct use TezClientConf.get(...).
Instead, it use as TezClientConf.someconf.key() and TezClientConf.someconf.defaultValue() where use Configuration.

eg: in RssDAGAppMaster.java

    // set the remote storage with actual value
    appMaster
        .getClusterClientConf()
        .put(TezClientConf.RSS_REMOTE_STORAGE_PATH.key(), remoteStorage.getPath());
    appMaster
        .getClusterClientConf()
        .put(TezClientConf.RSS_REMOTE_STORAGE_CONF.key(), remoteStorage.getConfString());

@zhengchenyu
Copy link
Collaborator

@lifeSo

We just need to reach some consensus. Then develop MR/Tez/Spark code based on this consensus.

This is my idea. Since TezConfiguration/SparkConf is start with "tez."/"spark.", then RssConfig is not start with "tez."/"spark.".
Then if TezClientConf is extended from RssConfig, TezClientConf/SparkConf should not start with "tez."/"spark.".

If so, TezClientConf should be extended from TezConfiguration. SparkClientConf should be extended from SparkConf.
RssClientConfig should be extended from RssConfig.
TezClientConf can be transient to RssClientConfig.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants