[#2455] feat(core): Support pluggable catalog operations #2477
Conversation
FANNG1
changed the title
FANNG1
force-pushed
the
operation
branch
6 times, most recently
from
7c64286 to
d0a1138
Compare
|
@jerryshao @mchades @yuqi1129 , it's ready to review now, please help to review when you are free. |
|
@TEOTEO520 please help to review when you are free. |
common/src/main/java/com/datastrato/gravitino/utils/PathUtils.java
Outdated
Show resolved
Hide resolved
...hive/src/test/java/com/datastrato/gravitino/catalog/hive/integration/test/CatalogHiveIT.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/datastrato/gravitino/catalog/TestBaseCatalog.java
Show resolved
Hide resolved
common/src/main/java/com/datastrato/gravitino/utils/PathUtils.java
Outdated
Show resolved
Hide resolved
...og-hadoop/src/main/java/com/datastrato/gravitino/catalog/hadoop/HadoopCatalogOperations.java
Show resolved
Hide resolved
core/src/main/java/com/datastrato/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
FANNG1
changed the title
|
@jerryshao , comment are addressed, please review again |
| } | ||
|
|
||
| @Override | ||
| public void initialize(Map<String, String> config) throws RuntimeException { | ||
| public void initialize(Map<String, String> config, CatalogEntity entity) throws RuntimeException { | ||
| this.entity = entity; |
There was a problem hiding this comment.
Could we introduce BaseCatalogOperations to extract common behavior like this?
There was a problem hiding this comment.
it's common to extract entity store, properties, but the main logic is catalog-specific, I'm not sure whether it's worth to do this.
|
Have you tested whether custom CatalogOperations can successfully pass end-to-end tests? |
good point, I could run pass HiveCatalogIT using custom catalog operations in local machine. |
jerryshao
changed the title
core/src/main/java/com/datastrato/gravitino/catalog/BaseCatalog.java
Outdated
Show resolved
Hide resolved
What changes were proposed in this pull request?
a. move
entityfrom XXCatalogOperation constructor to initialize(), to construct custom catalog operation using reflect easily.b. move CatalogOperation#initialize() from XXCatalogOperation#newOps() to BaseCatalog#Ops(), because initialize is the API, it should be called explicitly by the framework not by specific XXCatalogOperation.
Why are the changes needed?
Fix: #2455
Does this PR introduce any user-facing change?
no
How was this patch tested?
UT and IT