HIVE-27224: Enhance drop table/partition command #5851
Conversation
asf-ci-hive
added
tests pending
tests failed
and removed
tests pending
tests failed
labels
asf-ci-hive
added
tests unstable
tests pending
tests failed
and removed
tests pending
tests unstable
tests failed
labels
dengzhhu653
changed the title
asf-ci-hive
added
tests passed
tests pending
and removed
tests pending
tests passed
labels
|
|
A couple of test failures seem to be related to this patch. |
| wh.addToChangeManagement(funcCmPath); | ||
| } | ||
| if (req.isDeleteData()) { | ||
| // Moving the data deletion out of the async handler. |
There was a problem hiding this comment.
I think we should move this into the operation handler, because if a thrift client only calls this api once in async mode, then such cleanup code would never be run.
There was a problem hiding this comment.
In async mode, the client still need to ping the server for the operation status until the end, the client needs to know whether the request is a failure or not.
The main reason is the TUGIBasedProcessor/TUGIAssumingProcessor might close the shared FileSystem behind, causing the "java.io.IOException: Filesystem closed" for the handler running in background.
Still we need to address this "Filesystem closed" issue, as we don't know whether there are Filesystem operations in the Metastore listeners.
There was a problem hiding this comment.
FileSystem.closeAllForUGI(clientUgi); in TUGIAssumingProcessor seems a bug, assume that there are two requests with same ugi to handle the same path uri concurrently, it may also hit the "Filesystem closed" issue.
This is indeed a tricky problem, not sure if we can only remove cache for inactive ugi to solve it. And for this thread, it still has an issue if the client crush between two pings before the operation handler finished, the cleanup code will not take effect either.
There was a problem hiding this comment.
nice catch, we should take the client crash into the picture
...-server/src/main/java/org/apache/hadoop/hive/metastore/handler/AbstractOperationHandler.java
Show resolved
Hide resolved
dengzhhu653
force-pushed
the
HIVE-27224
branch
from
e603abc to
e54f5a9
Compare
asf-ci-hive
added
tests failed
tests pending
tests unstable
and removed
tests pending
tests failed
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
| if (ugiTransport.getClientUGI() == null) { | ||
| ugiTransport.setClientUGI(clientUgi); | ||
| } | ||
| clientUgi = ugiTransport.getClientUGI(); |
There was a problem hiding this comment.
Is this line unnecessary? clientUgi is already initialized.
There was a problem hiding this comment.
The ugi is identical: https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java#L483-L491,
reuse the ugi cached in ugiTransport if possible so the connection will get the same FileSystem instance from cache in the whole lifetime
...ne-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
Outdated
Show resolved
Hide resolved
| if (request.isNeedResult()) { | ||
| AddPartitionsHandler addPartsOp = AbstractOperationHandler.offer(this, request); | ||
| if (addPartsOp.success() && request.isNeedResult()) { | ||
| AddPartitionsHandler.AddPartitionsResult addPartsResult = addPartsOp.getResult(); |
There was a problem hiding this comment.
Can we store the partition list in the AddPartitionsResult and return it directly here?
There was a problem hiding this comment.
no enough, the addPartsOp.success() need to check on the state(success or not) of addPartsOp.getResult()
| if (async) { | ||
| OPID_CLEANER.schedule(() -> OPID_TO_HANDLER.remove(id), 1, TimeUnit.HOURS); | ||
| } | ||
| afterExecute(resultV); |
There was a problem hiding this comment.
If afterExecute() is needed only when the execute() is success, we can check the result here
| afterExecute(resultV); | |
| if (resultV != null && resultV.success()) { | |
| afterExecute(resultV); | |
| }``` |
There was a problem hiding this comment.
the afterExecute is also called in case of failure to free up some resources the handler might hold
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
tests unstable
labels
|
wecharyu
left a comment
wecharyu
left a comment
There was a problem hiding this comment.
LGTM(non-binding), it's a nice enhance to such heavy operations and a good start to split the huge HMSHandler class.
5 participants
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?