You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/05/18 15:16:57 UTC
[GitHub] [hive] belugabehr opened a new pull request #2291: RHIVE-25126: Remove Thrift Exceptions From RawStore alterCatalog
belugabehr opened a new pull request #2291:
URL: https://github.com/apache/hive/pull/2291
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #2291:
URL: https://github.com/apache/hive/pull/2291#discussion_r646590431
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaDataAccessException.java
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.metastore;
+
+/**
+ * Hive Exception when the metastore's underlying storage mechanism cannot be
+ * accessed.
+ */
+public class HiveMetaDataAccessException extends HiveMetaRuntimeException {
Review comment:
@nrg4878 Hey, thanks for the review.
I was looking at some other frameworks for inspiration here. I think the idea here is that even on a WRITE operation, if the database is down or there is a timeout to acquire the lock for writing, this is still an "access" exception. I would love to do some sub-classing and make more error specific to certain conditions, but I think the current naming is a good starting point.
https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/dao/DataAccessException.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #2291:
URL: https://github.com/apache/hive/pull/2291#discussion_r646596253
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -656,10 +656,9 @@ public void createCatalog(Catalog cat) throws MetaException {
}
@Override
- public void alterCatalog(String catName, Catalog cat)
- throws MetaException, InvalidOperationException {
+ public void alterCatalog(String catName, Catalog cat) {
if (!cat.getName().equals(catName)) {
- throw new InvalidOperationException("You cannot change a catalog's name");
+ throw new HiveMetaRuntimeException("You cannot change a catalog's name: " + cat.getName() + " -> " + catName);
Review comment:
@nrg4878 `InvalidOperationException` is a Thrift exception, so I'm trying to get it out of this class.
I am open to also making this a POJO `IllegalArgumentException` as well, but this would apply to quite a few methods so it probably should be done in another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] nrg4878 commented on a change in pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
nrg4878 commented on a change in pull request #2291:
URL: https://github.com/apache/hive/pull/2291#discussion_r646175377
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaRuntimeException.java
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.metastore;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * This is the root of all Hive Runtime Exceptions. If a client can reasonably
+ * be expected to recover from an exception, make it a checked exception. If a
+ * client cannot do anything to recover from the exception, make it an unchecked
+ * exception.
+ */
+public class HiveMetaRuntimeException extends RuntimeException {
Review comment:
is the plan to use this class as a parent for all runtime exceptions on the HMS? I am just wondering why we have HiveMetaRuntimeException and HiveMetaDataAccessRuntimeException? Will there be other sub-types for this class?
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -656,10 +656,9 @@ public void createCatalog(Catalog cat) throws MetaException {
}
@Override
- public void alterCatalog(String catName, Catalog cat)
- throws MetaException, InvalidOperationException {
+ public void alterCatalog(String catName, Catalog cat) {
if (!cat.getName().equals(catName)) {
- throw new InvalidOperationException("You cannot change a catalog's name");
+ throw new HiveMetaRuntimeException("You cannot change a catalog's name: " + cat.getName() + " -> " + catName);
Review comment:
Not a very enterprise-grade exception message. Should we re-word this to something like this? "Catalogs cannot be renamed"
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaDataAccessException.java
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.metastore;
+
+/**
+ * Hive Exception when the metastore's underlying storage mechanism cannot be
+ * accessed.
+ */
+public class HiveMetaDataAccessException extends HiveMetaRuntimeException {
Review comment:
nit: feels the class name feels incorrect
1) Suggests that this would be thrown on "access" operations aka read only operations. This is being thrown on an alterCatalog() operation which is WRITE operation. Would something like HiveMetaDataException be suitable for both READ+WRITE operations? (other considerations, HiveMetaDataException, HiveAccessException or just MetaDataRuntimeException)
2) Feels too long
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -656,10 +656,9 @@ public void createCatalog(Catalog cat) throws MetaException {
}
@Override
- public void alterCatalog(String catName, Catalog cat)
- throws MetaException, InvalidOperationException {
+ public void alterCatalog(String catName, Catalog cat) {
if (!cat.getName().equals(catName)) {
- throw new InvalidOperationException("You cannot change a catalog's name");
+ throw new HiveMetaRuntimeException("You cannot change a catalog's name: " + cat.getName() + " -> " + catName);
Review comment:
Should we distinguish between invalid operations/input arguments that are not permitted such as above vs runtime exceptions from backend DDL/DML operations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr closed pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr closed pull request #2291:
URL: https://github.com/apache/hive/pull/2291
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #2291:
URL: https://github.com/apache/hive/pull/2291#issuecomment-855934805
@nrg4878 This is a really heavy commit to change all the method signatures, though I do have another PR ready to go: #2290.
As you've seen from the initial interest in this patch, I didn't want to put the time into all of the work without some acknowledgement and buy-in on the current direction I'm trying to take this in. I still prefer the piecemeal approach for that reason. This PR is just a bit more foundational because it includes the Exceptions I would like to move forward with.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #2291:
URL: https://github.com/apache/hive/pull/2291#discussion_r646602792
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -656,10 +656,9 @@ public void createCatalog(Catalog cat) throws MetaException {
}
@Override
- public void alterCatalog(String catName, Catalog cat)
- throws MetaException, InvalidOperationException {
+ public void alterCatalog(String catName, Catalog cat) {
if (!cat.getName().equals(catName)) {
- throw new InvalidOperationException("You cannot change a catalog's name");
+ throw new HiveMetaRuntimeException("You cannot change a catalog's name: " + cat.getName() + " -> " + catName);
Review comment:
Thanks for the input on the error text. I've altered it a bit to make it more clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] github-actions[bot] closed pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #2291:
URL: https://github.com/apache/hive/pull/2291
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #2291:
URL: https://github.com/apache/hive/pull/2291#discussion_r646590431
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaDataAccessException.java
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.metastore;
+
+/**
+ * Hive Exception when the metastore's underlying storage mechanism cannot be
+ * accessed.
+ */
+public class HiveMetaDataAccessException extends HiveMetaRuntimeException {
Review comment:
@nrg4878 Hey, thanks for the review.
I was looking at some other frameworks for inspiration here. I think the idea here is that even on a WRITE operation, if the database is down or there is a timeout to acquire the lock for writing, this is still an "access" exception. I would love to do some sub-classing and make more error specific to certain conditions, but I think this is a good starting point.
https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/dao/DataAccessException.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr closed pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr closed pull request #2291:
URL: https://github.com/apache/hive/pull/2291
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr closed pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr closed pull request #2291:
URL: https://github.com/apache/hive/pull/2291
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #2291:
URL: https://github.com/apache/hive/pull/2291#issuecomment-846441655
@miklosgergely This one is an important foundation change based on my discussions in HIVE-25126, but the details for this particular PR are found in HIVE-25128.
Please let me know what you think.
Thanks!!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #2291:
URL: https://github.com/apache/hive/pull/2291#discussion_r646593817
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaRuntimeException.java
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.metastore;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * This is the root of all Hive Runtime Exceptions. If a client can reasonably
+ * be expected to recover from an exception, make it a checked exception. If a
+ * client cannot do anything to recover from the exception, make it an unchecked
+ * exception.
+ */
+public class HiveMetaRuntimeException extends RuntimeException {
Review comment:
@nrg4878 Yes. My hope would be to use this as the base class for all HMS Runtime Exceptions. DataAccess is just one such type which also serves to demonstrate the pattern I'm going for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] github-actions[bot] commented on pull request #2291: HIVE-25128: Remove Thrift Exceptions From RawStore alterCatalog
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #2291:
URL: https://github.com/apache/hive/pull/2291#issuecomment-894573562
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org