Skip to content

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

Closed
s0nskar wants to merge 7 commits intoapache:mainfrom
s0nskar:dynamic_config
Closed

[CELEBORN-1550] Add support of providing custom dynamic store backend implementation#2670
s0nskar wants to merge 7 commits intoapache:mainfrom
s0nskar:dynamic_config

Conversation

@s0nskar
Copy link
Contributor

@s0nskar s0nskar commented Aug 7, 2024

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
Copy link
Member

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

@s0nskar
Copy link
Contributor Author

s0nskar commented Aug 7, 2024

@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.

@s0nskar s0nskar changed the title Add support of providing custom dynamic store backend implementation [CELEBORN-1550] Add support of providing custom dynamic store backend implementation Aug 7, 2024
@s0nskar s0nskar marked this pull request as ready for review August 7, 2024 09:35
@AngersZhuuuu
Copy link
Contributor

AngersZhuuuu commented Aug 7, 2024

@s0nskar
Copy link
Contributor Author

s0nskar commented Aug 7, 2024

@AngersZhuuuu Updated the doc.

@SteNicholas
Copy link
Member

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

@s0nskar
Copy link
Contributor Author

s0nskar commented Aug 7, 2024

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

@SteNicholas
Copy link
Member

SteNicholas commented Aug 7, 2024

@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) { ...}
}

}
String configStoreBackendName = celebornConf.dynamicConfigStoreBackend().get();
String configStoreBackendClass =
dynamicConfigStoreBackendShortNames.getOrDefault(
Copy link
Member

@SteNicholas SteNicholas Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User could not add short name of the custom ConfigService implementation in dynamicConfigStoreBackendShortNames.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was sort of following what spark does for shuffle manager. It allows short names for sort and tungesten-sort but if user is passing custom shuffle manager then they have to pass fully qualified name.

@s0nskar
Copy link
Contributor Author

s0nskar commented Aug 8, 2024

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.

Copy link
Contributor

@waitinfuture waitinfuture left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Contributor

@RexXiong RexXiong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@waitinfuture
Copy link
Contributor

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

waitinfuture pushed a commit that referenced this pull request Aug 12, 2024
… implementation

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"

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

NO, user facing behaviour will be same.

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

Closes #2670 from s0nskar/dynamic_config.

Authored-by: Sanskar Modi <sanskarmodi97@gmail.com>
Signed-off-by: zky.zhoukeyong <zky.zhoukeyong@alibaba-inc.com>
(cherry picked from commit a0b04d0)
Signed-off-by: zky.zhoukeyong <zky.zhoukeyong@alibaba-inc.com>
@s0nskar s0nskar deleted the dynamic_config branch August 12, 2024 07:57
wankunde pushed a commit to wankunde/celeborn that referenced this pull request Oct 11, 2024
… implementation

### 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.

Closes apache#2670 from s0nskar/dynamic_config.

Authored-by: Sanskar Modi <sanskarmodi97@gmail.com>
Signed-off-by: zky.zhoukeyong <zky.zhoukeyong@alibaba-inc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants