[#4107] feat(all): Add testConnection API for catalog #4108
Conversation
api/src/main/java/com/datastrato/gravitino/exceptions/IllegalPropertyException.java
Outdated
Show resolved
Hide resolved
| return exception; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I have some comments on the API design:
- It is not necessary to have a new Interface, adding a new method in the existing interface should be fine.
testCatalogCreationis a big weird, I thinktestConnectionshould be OK.- The interface design is not Java-ish, it is more like a REST interface, especially
TestResult, it is totally DTO related stuff, which is weird for Java API design. For Java, it is better like
boolean testConnection(xxxx);
or
void testConnection(xxxx) throws Exception;
There was a problem hiding this comment.
Internally, you can design a such interface, but for API which is used by Java users, we should design to be more Java-ish.
There was a problem hiding this comment.
okay, using void testConnection(xxxx) throws Exception; to refactor the API, please help to review when you have time, thanks
mchades
changed the title
|
I'm fine with the API and REST interface. For the details, I need to take a deep look. |
| @@ -114,4 +114,23 @@ Catalog alterCatalog(String catalogName, CatalogChange... changes) | |||
| * @return True if the catalog was dropped, false otherwise. | |||
| */ | |||
| boolean dropCatalog(String catalogName); | |||
|
|
|||
| /** | |||
| * Test whether a catalog can be created successfully with the specified parameters, without | |||
There was a problem hiding this comment.
I believe, this should be 'Test whether we can connect to catalog engine(or something similar) with specified parameters'.
| String comment, | ||
| Map<String, String> properties) { | ||
| try { | ||
| adminClient.listTopics().names().get(); |
There was a problem hiding this comment.
Why do you use an already-created adminClient to test Connection since you intend to test connection with the parameter?
There was a problem hiding this comment.
The adminClient was initialized with the provided parameters
| return; | ||
| } | ||
|
|
||
| ErrorHandlers.catalogErrorHandler().accept(resp); |
There was a problem hiding this comment.
Why do need to call this again?
There was a problem hiding this comment.
To throw the corresponding exception
| } | ||
| }); | ||
| if (store.exists(ident, EntityType.CATALOG)) { | ||
| throw new CatalogAlreadyExistsException("Catalog %s already exists", ident); |
There was a problem hiding this comment.
Will it lead to test error?
There was a problem hiding this comment.
yes, whether the catalog name already exists is also a validation item.
There was a problem hiding this comment.
More precisely, this will cause the testConnection method to throw a CatalogAlreadyExistsException exception.
| new CatalogCreateRequest(catalogName, type, provider, comment, properties); | ||
| req.validate(); | ||
|
|
||
| ErrorResponse resp = |
There was a problem hiding this comment.
Why do you use ErrorResponse here, I found that it returns BaseResponse for normal scenario.
There was a problem hiding this comment.
Just one comment, can you please explain this @mchades
There was a problem hiding this comment.
It can be BaseResponse (test successfully) or ErrorResponse (test failed).
Since the ErrorResponse can contain all fields of BaseResponse (code field only), so I use ErrorResponse here. @jerryshao
There was a problem hiding this comment.
Can you please add a comment here?
What changes were proposed in this pull request?
Add testConnection API for catalog
Why are the changes needed?
Fix: #4107
Does this PR introduce any user-facing change?
yes, add a new API
How was this patch tested?
tests added