Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HIVE-25055: Improve the exception handling in HMSHandler #2218

Merged
merged 13 commits into from Jun 16, 2021
Merged

HIVE-25055: Improve the exception handling in HMSHandler #2218

merged 13 commits into from Jun 16, 2021

Conversation

dengzhhu653
Copy link
Member

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?

@dengzhhu653 dengzhhu653 reopened this Apr 24, 2021
@dengzhhu653 dengzhhu653 reopened this Apr 25, 2021
@belugabehr
Copy link
Contributor

Hello. I agree that this section of code needs a lot of help. However, I think the real issue is that we are throwing these Thrift-exceptions from deep in Hive core code. I propose that we stop doing that and instead wrap these Thrift exceptions appropriately as they bubble up from Hive-core. This is effort highlights yet another issue with working with these Thrift exceptions.

Take a look at https://issues.apache.org/jira/browse/HIVE-25126

@dengzhhu653
Copy link
Member Author

Hello. I agree that this section of code needs a lot of help. However, I think the real issue is that we are throwing these Thrift-exceptions from deep in Hive core code. I propose that we stop doing that and instead wrap these Thrift exceptions appropriately as they bubble up from Hive-core. This is effort highlights yet another issue with working with these Thrift exceptions.

Take a look at https://issues.apache.org/jira/browse/HIVE-25126

Hey, @belugabehr, thank you for the input. As I can see, remove the Thrift-exceptions from the Hive core is not easy. There are some external classes that implements the RawStore, for example, the authorizing RawStore, removing the exceptions will make some incompatible, the retrying Metastore client uses the exception to make a decision on retrying the current operation, it's may not easy to transform un-checked Exceptions into Thrift exceptions appropriately(by message?) and we should move the current throw-if-meet logic deep in hive core to the HMS Handler. What do you think?
Thanks.

vihangk1
vihangk1 reviewed Jun 2, 2021
View reviewed changes
Copy link
Contributor

@vihangk1 vihangk1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for taking this up. The patch at a high level looks good to me. I just left some comments/suggestions.

...astore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ExceptionHandler.java Show resolved Hide resolved
...tore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java Outdated
@@ -914,6 +914,7 @@ private boolean isViewTable(String catName, String dbName, String tblName) throw
long queryTime = doTrace ? System.nanoTime() : 0;
MetastoreDirectSqlUtils.timingTrace(doTrace, queryText, start, queryTime);
if (sqlResult.isEmpty()) {
query.closeAll();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is fixing a unrelated bug. Can we move this out of this PR and create a different JIRA for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to #2344

...tore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java Outdated
@@ -1453,6 +1454,7 @@ public ColumnStatistics getTableStats(final String catName, final String dbName,
};
List<Object[]> list = Batchable.runBatched(batchSize, colNames, b);
if (list.isEmpty()) {
b.closeAllQueries();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as earlier comment.

...astore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ExceptionHandler.java Outdated
return this;
}

public static TException rethrowException(Exception e) throws TException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

semantically the name rethrowException suggests we are throwing some exception, however we are returning exception here as well. This causes weird reading statements like throw rethrowException(e); Can we fix this by throwing MetaException instead of returning defaultMetaException()? Also, a comment for this method will be helpful since it not very easily understandable otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

...ne-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
@@ -667,15 +630,6 @@ public Configuration getConf() {
return conf;
}

private Map<String, String> getModifiedConf() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up. I sure looked confusing to me.

...astore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ExceptionHandler.java Outdated
* Throws if the input e is the instance of the class clzt or clze or clzc in order
*/
public <T extends Exception, E extends Exception, C extends Exception> ExceptionHandler
throwIfInstance(Class<T> t, Class<E> e, Class<C> c) throws T, E, C {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you have a 2 argument and 3 argument version of this. Could we improve this by
public ExceptionHandler throwIfInstance(Class ... te) throws T {
for (Class t : te) {
throwIfInstance(t);
}
return this;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can do it by this more simplified way, thank you very much for the comments!

@dengzhhu653
Copy link
Member Author

Hey @belugabehr, HIVE-25126 seems need a lot of changes, maybe we should push it step by step to accomplish that, what do you think?
Thanks.

@dengzhhu653
Copy link
Member Author

Hi, @vihangk1, would you mind taking another look if have secs? thanks! :)

vihangk1
vihangk1 approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@vihangk1 vihangk1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the suggested changes. Change looks good to me.

@vihangk1 vihangk1 merged commit 17e18c0 into apache:master Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vihangk1 vihangk1 vihangk1 approved these changes

Assignees
No one assigned
Labels
tests passed
Projects
None yet
Milestone
No milestone
4 participants
@dengzhhu653 @belugabehr @vihangk1 @kgyrtkirk