apache / celeborn

Apache Celeborn is an elastic and high-performance service for shuffle and spilled data.
https://celeborn.apache.org/
Apache License 2.0
862 stars 351 forks source link

[CELEBORN-1550] Add support of providing custom dynamic store backend implementation #2670

Closed s0nskar closed 1 month ago

s0nskar commented 1 month ago

What changes were proposed in this pull request?

Adding support of providing custom dynamic store backend implementation, users can now pass there own implementation for dynamic config store backend.

This change also keep the backwards compatibility of supporting short names for backend like "FS" and "DB"

Why are the changes needed?

Currently celeborn only supports File and DB based backend while there can be other ways of managing these configs.

Does this PR introduce any user-facing change?

NO, user facing behaviour will be same.

How was this patch tested?

Existing UTs verifies that this change is working for "FS" and "DB" implementation.

SteNicholas commented 1 month ago

@s0nskar, why is the implementation not based on SPI?

s0nskar commented 1 month ago

@SteNicholas We have an internal implementation of config management service which enforce authn/authz and constraints on specific configs. It also helps us manage the rollout better between different regions.

AngersZhuuuu commented 1 month ago

Should also update doc here https://celeborn.apache.org/docs/latest/developers/configuration/#dynamic-configuration

s0nskar commented 1 month ago

@AngersZhuuuu Updated the doc.

SteNicholas commented 1 month ago

@s0nskar, ConfigService should introduce getName() interface for short name configuration.

s0nskar commented 1 month ago

@SteNicholas Added. Q: do you want me to make use of it in this PR?

SteNicholas commented 1 month ago

@s0nskar, the implementation of ConfigService could use getName interface to load the implementation instance instead of hard code in factory. Or you could regard DynamicConfigServiceFactory as interface and the getName interface could be introduced in DynamicConfigServiceFactory.

public interface DynamicConfigServiceFactory {
   public String getName();

   public ConfigService getConfigService(CelebornConf celebornConf);
}

public class FSConfigServiceFactory implements DynamicConfigServiceFactory {
   public String getName() { return "FS";}
   public ConfigService getConfigService(CelebornConf celebornConf) { ...}
}
s0nskar commented 1 month ago

the implementation of ConfigService could use getName interface to load the implementation instance instead of hard code in factory.

@SteNicholas Trying to understand this comment better – I think for this will have to make getName() static method but abstract static does not work.

The factory approach will also need a lot of abstraction IMO it is not necessary for this as we will only have couple of predefined names. Let me know if you think otherwise then i will try to make it work.

waitinfuture commented 1 month ago

Merging to main(v0.6.0)/branch-0.5(v0.5.2)