Throw table offline exception when object not readable #22752
Conversation
NikhilCollooru
force-pushed
the
tableOffline
branch
2 times, most recently
from
08dcf8b
to
ddc7b00
Compare
presto-hive/src/main/java/com/facebook/presto/hive/HivePartitionManager.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/com/facebook/presto/hive/HiveWriteUtils.java
Outdated
Show resolved
Hide resolved
| @@ -478,6 +479,12 @@ public static void verifyOnline(SchemaTableName tableName, Optional<String> part | |||
| throw new TableOfflineException(tableName, false, null); | |||
| } | |||
|
|
|||
| // Throw table offline exception for un-partitioned tables when table parameters has object_not_readable | |||
| String objectNotReadable = parameters.get(OBJECT_NOT_READABLE); | |||
| if (!isNullOrEmpty(objectNotReadable) && !isPartitioned) { | |||
There was a problem hiding this comment.
I'm curious why the !partitioned check needed. What if table is partitioned and marked as OBJECT_NOT_READABLE?
There was a problem hiding this comment.
There are two scenarios
- Table has not readable flag and only of 1 of the N partitions of the table are not readable
- Table has not readable flag and ALL of the partitions are also readable.
So for a partitioned table, I am not sure what object notreadble flag mean in table parameters.
For un-partitioned table, if not readable flag exists...its pretty clear that the table is not readable..because no partitions exist. Hence added this check.
NikhilCollooru
force-pushed
the
tableOffline
branch
from
ddc7b00
to
84b0f65
Compare
NikhilCollooru
force-pushed
the
tableOffline
branch
2 times, most recently
from
d84a698
to
d84e827
Compare
elharo
dismissed
their stale review
looks like tests might be passing now, rerunning
There was a problem hiding this comment.
There's still a related test failure:
Error: Tests run: 2981, Failures: 1, Errors: 0, Skipped: 90, Time elapsed: 2,804.529 s <<< FAILURE! - in TestSuite
Error: com.facebook.presto.hive.TestHiveClientFileMetastore.testUnPartitionedTableNotReadable Time elapsed: 0.004 s <<< FAILURE!
java.lang.NullPointerException
at com.facebook.presto.hive.HiveTableProperties.getBucketProperty(HiveTableProperties.java:206)
at com.facebook.presto.hive.HiveMetadata.prepareTable(HiveMetadata.java:1001)
at com.facebook.presto.hive.HiveMetadata.createTable(HiveMetadata.java:975)
at com.facebook.presto.hive.TestHiveClientFileMetastore.testUnPartitionedTableNotReadable(TestHiveClientFileMetastore.java:132)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:135)
at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:673)
at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:220)
at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:945)
at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:193)
at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:750)
|
@NikhilCollooru has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
NikhilCollooru
force-pushed
the
tableOffline
branch
from
d84e827
to
2542d64
Compare
|
@NikhilCollooru has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Not sure if test failure is related or not
Error: Tests run: 2982, Failures: 1, Errors: 0, Skipped: 90, Time elapsed: 2,960.526 s <<< FAILURE! - in TestSuite
Error: com.facebook.presto.hive.TestHiveClientFileMetastore.cleanup Time elapsed: 0.031 s <<< FAILURE!
com.facebook.presto.spi.PrestoException: Database test is not empty
at com.facebook.presto.hive.metastore.file.FileHiveMetastore.dropDatabase(FileHiveMetastore.java:194)
at com.facebook.presto.hive.AbstractTestHiveClientLocal.cleanup(AbstractTestHiveClientLocal.java:81)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at
|
Actually this is tricky. Now its failing when doing getTableHandle during the drop and we are throwing exception. We should probably throw the tableOfflineException only when the query is trying to read from table. |
| @@ -308,6 +308,7 @@ public static void checkPartitionIsWritable(String partitionName, Partition part | |||
| public static void checkWritable( | |||
| SchemaTableName tableName, | |||
| Optional<String> partitionName, | |||
| boolean isPartitioned, | |||
Description
We need to throw table offline exception when the table parameters contain object not readable parameter set.
Motivation and Context
Currently for unpartitioned tables, we are not throwing table offline exception even when object_not_readable key is set in table parameters. Unlike how we throw exception when this key is present for partition parameters.
Impact
None
Test Plan
exisiting tests
Contributor checklist
Release Notes