Skip to content

[CELEBORN-1521][FOLLOWUP] Move BasicPrincipal to spi module#2665

Closed
turboFei wants to merge 2 commits intoapache:mainfrom
turboFei:spi_follow
Closed

[CELEBORN-1521][FOLLOWUP] Move BasicPrincipal to spi module#2665
turboFei wants to merge 2 commits intoapache:mainfrom
turboFei:spi_follow

Conversation

@turboFei
Copy link
Member

@turboFei turboFei commented Aug 5, 2024

What changes were proposed in this pull request?

This PS is a followup of CELEBORN-1521, move the BasicPrincipal in to celeborn-spi module. so that customer do not need to implement it by themselves.

Why are the changes needed?

For authentication extension.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing GA.

@turboFei turboFei changed the title Remove celebornConf constructor [CELEBORN-1521][FOLLOWUP] Move BasicPrincipal to spi module Aug 5, 2024
@turboFei turboFei changed the title [CELEBORN-1521][FOLLOWUP] Move BasicPrincipal to spi module [CELEBORN-1521][FOLLOWUP] Move BasicPrincipal to spi module and remove unreachable constructor in authentication factory Aug 5, 2024
@turboFei
Copy link
Member Author

turboFei commented Aug 5, 2024

cc @pan3793

@pan3793
Copy link
Member

pan3793 commented Aug 6, 2024

move the BasicPrincipal in to celeborn-spi module

I'm not against this, but does it have benefits?

remove the constructor with CelebornConf in HttpAuthenticationFactory as CelebornConf is in celeborn-common module and not deployed for developer.

I would keep this, the developer still has the option to write custom plugins in their forked celeborn repo

@turboFei
Copy link
Member Author

turboFei commented Aug 6, 2024

move the BasicPrincipal in to celeborn-spi module

I'm not against this, but does it have benefits?

The only benefit is that, customer do not need to implement java.security.Principal by them, that is all

@pan3793
Copy link
Member

pan3793 commented Aug 6, 2024

The only benefit is that, customer do not need to implement java.security.Principal by them, that is all

sounds reasonable

@turboFei turboFei changed the title [CELEBORN-1521][FOLLOWUP] Move BasicPrincipal to spi module and remove unreachable constructor in authentication factory [CELEBORN-1521][FOLLOWUP] Move BasicPrincipal to spi module Aug 6, 2024
@pan3793 pan3793 closed this in 6d2e1c8 Aug 7, 2024
@pan3793
Copy link
Member

pan3793 commented Aug 7, 2024

Merged to main

wankunde pushed a commit to wankunde/celeborn that referenced this pull request Oct 11, 2024
### What changes were proposed in this pull request?

This PS is a followup of CELEBORN-1521, move the BasicPrincipal in to celeborn-spi module. so that customer do not need to implement it by themselves.
### Why are the changes needed?

For authentication extension.

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

### How was this patch tested?

Existing GA.

Closes apache#2665 from turboFei/spi_follow.

Authored-by: Wang, Fei <fwang12@ebay.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
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.

2 participants