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.

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

[Improvement] Optimize error message when alter an unknown table #555

Closed
yuqi1129 opened this issue Oct 19, 2023 · 0 comments · Fixed by #556
Closed

[Improvement] Optimize error message when alter an unknown table #555

yuqi1129 opened this issue Oct 19, 2023 · 0 comments · Fixed by #556
Assignees
Labels
improvement Improvements on everything

Comments

@yuqi1129
Copy link
Contributor

What would you like to be improved?

Error message of the following test is not proper:

  @Test
  void testAlterUnknownTable() {
    NameIdentifier identifier = NameIdentifier.of(metalakeName, catalogName, schemaName, "unknown");
    Assertions.assertThrows(
        NoSuchTableException.class,
        () -> {
          catalog.asTableCatalog().alterTable(identifier, TableChange.updateComment("new_comment"));
        });
  }

And message is:

2023-10-19 16:41:07 INFO  PrintFuncNameExtension:18 - ===== Entry test: testAlterUnknownTable() =====
2023-10-19 16:41:07 WARN  ExceptionHandlers:58 - Failed to operate table(s) [unknown] operation [ALTER] under schema [CatalogHiveIT_schema_4116338773b547e18d832d49782a363f], reason [RuntimeException]
java.lang.RuntimeException: java.lang.RuntimeException: com.datastrato.gravitino.exceptions.NoSuchTableException: Hive table does not exist: unknown in Hive Metastore
	at com.datastrato.gravitino.catalog.CatalogOperationDispatcher.doWithCatalog(CatalogOperationDispatcher.java:611) ~[gravitino-core-0.2.0-SNAPSHOT.jar:?]
	at com.datastrato.gravitino.catalog.CatalogOperationDispatcher.alterTable(CatalogOperationDispatcher.java:431) ~[gravitino-core-0.2.0-SNAPSHOT.jar:?]
	at com.datastrato.gravitino.server.web.rest.TableOperations.alterTable(TableOperations.java:136) ~[gravitino-server-0.2.0-SNAPSHOT.jar:?]
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_372]
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_372]
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_372]
	at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_372]
	at org.glassfish.jersey.server.model.internal.ResourceMethodInvocationHandlerFactory.lambda$static$0(ResourceMethodInvocationHandlerFactory.java:52) ~[jersey-server-2.39.1.jar:?]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher$1.run(AbstractJavaResourceMethodDispatcher.java:134) ~[jersey-server-2.39.1.jar:
...

I think we should throw NoSuchTableException instead of a wrapper exception RuntimeException.

How should we improve?

No response

@yuqi1129 yuqi1129 added the improvement Improvements on everything label Oct 19, 2023
@yuqi1129 yuqi1129 changed the title [Improvement] Optimize error message when alter a unknown table [Improvement] Optimize error message when alter an unknown table Oct 19, 2023
@yuqi1129 yuqi1129 self-assigned this Oct 19, 2023
jerryshao pushed a commit that referenced this issue Oct 19, 2023
…nown table (#556)

### What changes were proposed in this pull request?

Change the exception class from `RuntimeException` to
`NoSuchTableException` when altering an unknown table.

### Why are the changes needed?

Improving error messages to be more accurate and readable.

Fix: #555 

### Does this PR introduce _any_ user-facing change?

NO

### How was this patch tested?

Add test int IT.
jerryshao pushed a commit that referenced this issue Oct 26, 2023
…nown table (#556)

### What changes were proposed in this pull request?

Change the exception class from `RuntimeException` to
`NoSuchTableException` when altering an unknown table.

### Why are the changes needed?

Improving error messages to be more accurate and readable.

Fix: #555 

### Does this PR introduce _any_ user-facing change?

NO

### How was this patch tested?

Add test int IT.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvements on everything
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant