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] Make config type of RSS_STORAGE_TYPE as enum #299

Closed
2 of 3 tasks
zuston opened this issue Nov 5, 2022 · 4 comments · Fixed by #1052 or #1123
Closed
2 of 3 tasks

[Improvement] Make config type of RSS_STORAGE_TYPE as enum #299

zuston opened this issue Nov 5, 2022 · 4 comments · Fixed by #1052 or #1123
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@zuston
Copy link
Member

zuston commented Nov 5, 2022

Code of Conduct

Search before asking

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

What would you like to be improved?

Make config type of RSS_STORAGE_TYPE in RssBaseConf as enum

How should we improve?

The enum type of config option is introduced in #199, follow this PR to fix the storage type config

Are you willing to submit PR?

  • Yes I am willing to submit a PR!
@zuston zuston added good first issue Good for newcomers help wanted Extra attention is needed labels Nov 5, 2022
@selectbook
Copy link
Contributor

is it change to something like this?

public static final ConfigOption RSS_STORAGE_TYPE = ConfigOptions
.key("rss.storage.type")
.enumType(StorageType.class)
.noDefaultValue()
.withDescription("Data storage for remote shuffle service");

@jerqi
Copy link
Contributor

jerqi commented Nov 9, 2022

is it change to something like this?

public static final ConfigOption RSS_STORAGE_TYPE = ConfigOptions .key("rss.storage.type") .enumType(StorageType.class) .noDefaultValue() .withDescription("Data storage for remote shuffle service");

Yes, do you want to submit a PR?

@selectbook
Copy link
Contributor

is it change to something like this?
public static final ConfigOption RSS_STORAGE_TYPE = ConfigOptions .key("rss.storage.type") .enumType(StorageType.class) .noDefaultValue() .withDescription("Data storage for remote shuffle service");

Yes, do you want to submit a PR?

yes i will fix this

@amaliujia
Copy link
Contributor

I looked this a bit. I thought this requires a good amount of refactoring. For example you might need to move StorageType.class to common module.

cchung100m added a commit to cchung100m/incubator-uniffle that referenced this issue Jul 30, 2023
cchung100m added a commit to cchung100m/incubator-uniffle that referenced this issue Jul 30, 2023
cchung100m added a commit to cchung100m/incubator-uniffle that referenced this issue Jul 31, 2023
cchung100m added a commit to cchung100m/incubator-uniffle that referenced this issue Jul 31, 2023
cchung100m added a commit to cchung100m/incubator-uniffle that referenced this issue Jul 31, 2023
cchung100m added a commit to cchung100m/incubator-uniffle that referenced this issue Jul 31, 2023
zuston pushed a commit that referenced this issue Aug 7, 2023
### What changes were proposed in this pull request?

improvement: Make config type of `RSS_STORAGE_TYPE` in `RssBaseConf` as enum

### Why are the changes needed?

Fix: #299

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

current UT and integration testing
jerqi added a commit that referenced this issue Aug 7, 2023
jerqi added a commit that referenced this issue Aug 7, 2023
cchung100m added a commit to cchung100m/incubator-uniffle that referenced this issue Aug 8, 2023
cchung100m added a commit to cchung100m/incubator-uniffle that referenced this issue Aug 8, 2023
cchung100m added a commit to cchung100m/incubator-uniffle that referenced this issue Aug 8, 2023
jerqi pushed a commit that referenced this issue Aug 8, 2023
### What changes were proposed in this pull request?

improvement: Make config type of `RSS_STORAGE_TYPE` in `RssBaseConf` as enum

### Why are the changes needed?

Fix: #299 and re-submit for #1106

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

current UT and integration testing
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 help wanted Extra attention is needed
Projects
None yet
4 participants