[#2999][#2997] improvement(partition): add partition operation dispatcher #3221
Conversation
| cl -> { | ||
| Preconditions.checkArgument( | ||
| asTables() != null, "Catalog does not support table operations"); | ||
| Table table = asTables().loadTable(tableIdent); |
There was a problem hiding this comment.
Can we load table first and then as a parameter to this method? So we would not load the table each time.
There was a problem hiding this comment.
Yes, but there is a trade-off:
- load table first and then as a parameter to this method: load table once, switch thread class loader 4 times(2 for load table, 2 for partition ops)
- current implementation: load table once (Since each request will only perform one partition operation currently), switch thread class loader 2 times
Therefore, I think it's fine for the current implementation, WDYT?
There was a problem hiding this comment.
The current implementation is fine as is, and we can change it after the full test is completed.
core/src/main/java/com/datastrato/gravitino/catalog/PartitionDispatcher.java
Show resolved
Hide resolved
|
@jerryshao All comments are resolved, do you want to take another look? |
|
Yeah, I will. |
|
How do you verify that the partition operation is running in the isolated classloader, is it possible to add the test case to cover this? |
@jerryshao test for isolated classloader added, please help review again |
| asTables() != null, "Catalog does not support table operations"); | ||
| Table table = asTables().loadTable(tableIdent); | ||
| Preconditions.checkArgument( | ||
| table.supportPartitions() != null, "Table does not support partition operations"); |
There was a problem hiding this comment.
Is it throwing an exception (UnsupportedOperationException) or returning null?
There was a problem hiding this comment.
UnsupportedOperationException
There was a problem hiding this comment.
So should I remove this check?
There was a problem hiding this comment.
Perhaps it can avoid the implementer returning null.
There was a problem hiding this comment.
if it will never return null, you don't need to handle this, otherwise, you need to think about how to handle UnsupportedOperationException
There was a problem hiding this comment.
if it will never return null, you don't need to handle this,
Whether null will be returned depends on the developer of the catalog. The current implementation is more like defensive programming.
you need to think about how to handle UnsupportedOperationException
Since UnsupportedOperationException is a RuntimeException, I think we could just throws it out
…cher (#3221) ### What changes were proposed in this pull request? - reuse the class loader of the catalog for partition operation - add partition operation dispatcher ### Why are the changes needed? Fix: #2997 #2999 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests added
…ation dispatcher (apache#3221) ### What changes were proposed in this pull request? - reuse the class loader of the catalog for partition operation - add partition operation dispatcher ### Why are the changes needed? Fix: apache#2997 apache#2999 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests added
What changes were proposed in this pull request?
Why are the changes needed?
Fix: #2997 #2999
Does this PR introduce any user-facing change?
no
How was this patch tested?
tests added