[Improvement] Use ConfigBuilder to rewrite the class RssSparkConfig

[smallzhongfeng]

What changes were proposed in this pull request?

Call Spark's class ConfigBuilder to optimize the original code.

Why are the changes needed?

You can annotate these parameters and make the code more standardized.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Use the original test.

[codecov-commenter]

Codecov Report

Merging #104 (c0937b5) into master (4e4b940) will increase coverage by 0.64%.
The diff coverage is 93.28%.

@@             Coverage Diff              @@
##             master     #104      +/-   ##
============================================
+ Coverage     56.45%   57.10%   +0.64%     
- Complexity     1182     1188       +6     
============================================
  Files           149      149              
  Lines          8008     8129     +121     
  Branches        767      767              
============================================
+ Hits           4521     4642     +121     
- Misses         3243     3244       +1     
+ Partials        244      243       -1     

Impacted Files	Coverage Δ
...rg/apache/uniffle/client/util/RssClientConfig.java	0.00% <ø> (ø)
...org/apache/spark/shuffle/RssSparkShuffleUtils.java	49.20% <77.77%> (ø)
...che/spark/shuffle/writer/BufferManagerOptions.java	75.67% <78.94%> (+1.48%)	⬆️
.../java/org/apache/spark/shuffle/RssSparkConfig.java	96.29% <97.16%> (+21.29%)	⬆️
...a/org/apache/uniffle/server/ShuffleServerConf.java	99.15% <0.00%> (+0.02%)	⬆️
...he/uniffle/server/storage/LocalStorageManager.java	61.90% <0.00%> (+5.90%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

[jerqi]

We find that we can rewrite the method converter to create a ConfigEntry instead of ConfigEntry.
The simple code is as below.

  public static final scala.Function1<String, Integer> f =
      x -> ConfigUtils.convertValue(x, Integer.class);

  void test() {
    ConfigBuilder builder = new ConfigBuilder(RSS_PARTITION_NUM_PER_RANGE);
    TypedConfigBuilder tbuilder = new TypedConfigBuilder<Integer>(builder, f);
    new SparkConf().get(tbuilder.createWithDefault(1));
    //    .createWithDefault(Integer.valueOf(1));
    //new SparkConf().getInt(entry);
        // SparkConf conf = new SparkConf();
    // int x = conf.get(RSS_PARTITION_NUM_PER_RANGE_ENTRY)
  }

Maybe we can add a method like

TypeConfigBuilder<T> typeBuilder<T>(ConfigBuilder builder) {
       scala.Function1<String, Integer> f =
            x -> ConfigUtils.convertValue(x, T.class);
       return new new TypedConfigBuilder<Integer>(builder, f);
}

[smallzhongfeng]

How to implement this method convertValue ? Could you show the code in detail? I have tried similar methods before, but I always failed when compiling. So I'm not sure if this is a better way.

[jerqi]

How to implement this method convertValue ? Could you show the code in detail? I have tried similar methods before, but I always failed when compiling. So I'm not sure if this is a better way.

Just modify some code , i pass the compile, the code is as below

  public static final scala.Function1<String, Integer> f = new SerializableFunction1<String, Integer>() {
    public Integer apply(String in){
      return ConfigUtils.convertValue(in, Integer.class);
    }
  };


  void test() {
    ConfigBuilder builder = new ConfigBuilder(RSS_PARTITION_NUM_PER_RANGE);
    TypedConfigBuilder tbuilder = new TypedConfigBuilder<Integer>(builder, f);
    new SparkConf().get(tbuilder.createWithDefault(1));
    //    .createWithDefault(Integer.valueOf(1));
    //new SparkConf().getInt(entry);
        // SparkConf conf = new SparkConf();
    // int x = conf.get(RSS_PARTITION_NUM_PER_RANGE_ENTRY)
  }

  public static abstract class SerializableFunction1<T1,R> extends AbstractFunction1<T1, R> implements Serializable {
  }

ConfigUtils.convertValue is our Uniffle project method. We can find it in common module.
@smallzhongfeng

[smallzhongfeng]

public static ConfigEntry<Integer> RSS_PARTITION_NUM_PER_RANGE = typeBuilder(
      new ConfigBuilder("spark.rss.partitionNum.per.range")
          .doc("xxxxxx"))
      .createWithDefault(RssClientConfig.RSS_PARTITION_NUM_PER_RANGE_DEFAULT_VALUE);

Is that what you mean? @jerqi

[jerqi]

public static ConfigEntry<Integer> RSS_PARTITION_NUM_PER_RANGE = typeBuilder(
      new ConfigBuilder("spark.rss.partitionNum.per.range")
          .doc("xxxxxx"))
      .createWithDefault(RssClientConfig.RSS_PARTITION_NUM_PER_RANGE_DEFAULT_VALUE);

Is that what you mean? @jerqi

Yes, that is. And could you give typeBuilder a better name? createTypedBuilder?

[jerqi]

You can refer to https://github.com/apache/incubator-uniffle/blob/master/docs/client_guide.md about some configuration description.

[smallzhongfeng]

scala.Function1<String, T> f =
            x -> ConfigUtils.convertValue(x, T.class);

Here, it is not supported to convert variables of String type into data of Generic type, so we may need to provide typeBuilder methods of Integer, Long, Boolean, etc

[jerqi]

scala.Function1<String, T> f =
            x -> ConfigUtils.convertValue(x, T.class);

Here, it is not supported to convert variables of String type into data of Generic type, so we may need to provide typeBuilder methods of Integer, Long, Boolean, etc

Maybe we can use the code like

scala.Function1<String, T>  createConvertFunction(Class class) {
  scala.Function1<String, T> f =
            x -> ConfigUtils.convertValue(x, class);
  return f;
}

[jerqi]

If we don't have accurate description, we remove doc first.

[smallzhongfeng]

done.

[jerqi]

Except for https://github.com/apache/incubator-uniffle/pull/104/files#r933988660, LGTM, @frankliee Do you have any other suggestion?

[smallzhongfeng]

cc @frankliee @jerqi

[jerqi]

+1, LGTM

[frankliee]

cc @frankliee @jerqi

LGTM too

[jerqi]

Merged, @smallzhongfeng thanks for your contribution.

[smallzhongfeng]

Thanks for your support.@jerqi