[#5842] feat(core): supports credential REST endpoint in Gravitino server #5841
Conversation
FANNG1
force-pushed
the
credential-server
branch
from
fb3859c to
cbbd277
Compare
FANNG1
changed the title
|
@yuqi1129 @jerryshao please help to review , thanks |
| new CredentialResponse( | ||
| DTOConverters.toDTO(credentials.toArray(new Credential[credentials.size()])))); | ||
| }); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Are we sure that this exception will not contain any sensitive information?
The control flow shows that some of these information are logged and printed.
There was a problem hiding this comment.
yes, the sensitive credential information is not logged.
| "Load credentials is not implemented for catalog: %s, identifier: %s", | ||
| catalog.name(), identifier)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we need catalog to load credentials?
There was a problem hiding this comment.
- use catalog classloader to load credential packages
- use catalog operations to load fileset, schema, and catalog properties to choose the correct credential provider.
|
@jerryshao any other comments? |
|
My feeling is that this XXXOperationDispatcher is being obsessively used everywhere, I don't see a strong reason to use it, I would suggest to avoid this dispatcher pattern if unnecessary. |
|
If you want to use the method in |
refactored with |
What changes were proposed in this pull request?
add credential REST endpoint in Gravitino server
Why are the changes needed?
Fix: #5842
Does this PR introduce any user-facing change?
no
How was this patch tested?
add UT and run test in POC