[#5760][#5780] fix(catalog): fix drop catalog error #5761
Conversation
| } | ||
|
|
||
| private void createSchema() { | ||
| private void createSchema(Catalog catalog, String schemaName) { |
There was a problem hiding this comment.
I don't think you need to pass Catalog here.
createSchema() is a private function, and it can access catalog anyway.
There was a problem hiding this comment.
because the method also used to create a schema under a specific catalog, see the 267 line in testDropCatalog()
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
|
|
||
| private boolean haveAvailableSchemas( |
There was a problem hiding this comment.
| private boolean haveAvailableSchemas( | |
| private boolean hasSchemas( |
There was a problem hiding this comment.
how about hasAvailableSchemas?
There was a problem hiding this comment.
I assume that the "default" schema is not an available schema in this method
| return false; | ||
| } | ||
|
|
||
| if (schemaEntities.size() == 1 && catalogEntity.getProvider().equals("kafka")) { |
There was a problem hiding this comment.
This can be optimized, IMHO.
If kafka is special for a reason, e.g. kafka is evil, you may want to add a function
to the class isProviderEvil(). I mean, you may want to consolidate this kind of tests
based on the business logic and maintain that logic, rather than treating kafka as
a special animal.
There was a problem hiding this comment.
Currently, only the Kafka catalog has the feature of having only one schema (named default). Therefore, I think it may not be a compelling enough reason to refactor at the API level.
But extra a method such as inRawStatus(CatalogEntity, List<SchemaEntity>) sounds a good idea, WDYT?
There was a problem hiding this comment.
so ... do we have that 'default' schema recorded as we do for other catalogs?
We can remove this if () condition if so.
In theory, the default schema is still a schema. Returning false is not a good idea. And this shortcut complicates the code.
I was striving to reduce hard-coded specializations in the code.
There was a problem hiding this comment.
so ... do we have that 'default' schema recorded as we do for other catalogs?
We can remove this if () condition if so.
In theory, the default schema is still a schema. Returning false is not a good idea. And this shortcut complicates the code.
this function is hasAvailableSchemas not hasSchemas, the "default" schema of Kafka catalog in this function is not treated as an available schema.
I was striving to reduce hard-coded specializations in the code.
The Catalog API is user-oriented. If we were to add a method like isProviderEvil() to the API, it might confuse users and require additional explanation. It would be more costly than adding an extra method such as inRawStatus(CatalogEntity, List<SchemaEntity>) in this class.
There was a problem hiding this comment.
If we are still unable to reach a consensus, I would like to hear the opinions of others. WDYT? @jerryshao @yuqi1129
There was a problem hiding this comment.
BTW, I will also add a condition here catalogEntity.getProvider().equals("jdbc-postgresql") && schemaEntities.size() == 1 && "public".equals(schemaEntities.get(0)) to resolve issue #5780
There was a problem hiding this comment.
I think we can have a better name for this method and add more comments to explain the logic, in the meantime we can split out the special Kafka logic to make it more understandable.
There was a problem hiding this comment.
let me add more comments on this method
There was a problem hiding this comment.
I have updated the method, plz help to review it again when you have time, thx! @tengqm @jerryshao
mchades
changed the title
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
| * otherwise. | ||
| * @throws Exception If an error occurs while checking the schemas. | ||
| */ | ||
| private boolean containsUserCreatedSchemas( |
There was a problem hiding this comment.
How about change to containsExternalSchemas or containsSchemasFromExternalCatalog?
There was a problem hiding this comment.
Both of these options are not entirely accurate. Since the "public" schema in the PG catalog is an external schema (not created by Gravitino), the correct return here should be false.
There was a problem hiding this comment.
From my feeling, how do we define "UserCreated..."? Both schemas created w/ or w/o Gravitino can be called "user created...", it is hard to understand from the method name.
There was a problem hiding this comment.
Basically all schemas are created by users, except:
- "public" schema in PG is created by PG itself, not a user-created schema
- "default" schema in kafka catalog is created by Kafka catalog itself, not a user-created schema
This is unrelated to whether it was created by Gravitino, but rather pertains to whether it was created by a user.
| } catch (GravitinoRuntimeException e) { | ||
| throw e; | ||
|
|
||
| } catch (Exception e) { |
There was a problem hiding this comment.
Why do we treat them differently?
There was a problem hiding this comment.
Because if we wrap the GravitinoRuntimeException to RuntimeException , the error response will be aways INTERNAL_SERVER_ERROR, the CatalogExceptionHandler can not convert it to correspond error response
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
| * otherwise. | ||
| * @throws Exception If an error occurs while checking the schemas. | ||
| */ | ||
| private boolean containsUserCreatedSchemas( |
There was a problem hiding this comment.
From my feeling, how do we define "UserCreated..."? Both schemas created w/ or w/o Gravitino can be called "user created...", it is hard to understand from the method name.
What changes were proposed in this pull request?
before drop the catalog, check all schemas are avaliable
Why are the changes needed?
some schemas are dropped externally, but still exist in the entity store, those schemas are invalid
Fix: #5760
Fix: #5780
Does this PR introduce any user-facing change?
no
How was this patch tested?
CI pass