HIVE-19970 : Replication dump has a NPE when table is empty #379
Conversation
| } catch (RuntimeException e) { | ||
| LOG.error("failed", e); | ||
| setException(e); | ||
| return ErrorMsg.getErrorMsg(e.getMessage()).getErrorCode(); |
There was a problem hiding this comment.
Do we have error code set for all instances of RuntimeException?
There was a problem hiding this comment.
we are just using the message ..the message will be set by the method throwing the excpetion and for the messages which are not set ..get error code returns generic error
| for (Future<?> future : futures) { | ||
| try { | ||
| future.get(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
It is better to catch only RuntimeException as otherwise it may suppress others.
There was a problem hiding this comment.
it was suppressed earlier ..now we are getting the exceptions and throwing it explicitly
| @@ -4986,6 +4986,10 @@ private void alter_table_core(final String catName, final String dbname, final S | |||
| try { | |||
| Table oldt = get_table_core(catName, dbname, name); | |||
| firePreEvent(new PreAlterTableEvent(oldt, newTable, this)); | |||
| if (newTable.getDbName() == null) { | |||
There was a problem hiding this comment.
In what use case the new DB name can be null?
There was a problem hiding this comment.
its just for making the exception same ..as per review comment from vary
| @@ -907,7 +907,7 @@ public void testAlterTableCascade() throws Exception { | |||
| } | |||
| } | |||
|
|
|||
| @Test(expected = MetaException.class) | |||
| @Test(expected = InvalidOperationException.class) | |||
There was a problem hiding this comment.
Why is this behaviour change?
There was a problem hiding this comment.
its just to revert back the changes ..not sure if its required or not so will remove this change
| future.get(); | ||
| } catch (Exception e) { | ||
| LOG.error("failed", e.getCause()); | ||
| throw new HiveException(e.getCause().getMessage(), e.getCause()); |
There was a problem hiding this comment.
Why do we need this special handling for PartitionExport with futures semantics? FileOperations.export itself take care of NPE when partition directory missing right?
There was a problem hiding this comment.
FileOperations.export done it a separate thread so it has to be done manually here
| @@ -3185,6 +3186,27 @@ public void testLoadCmPathMissing() throws IOException { | |||
| fs.create(path, false); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testDumpFileMissing() throws IOException { | |||
There was a problem hiding this comment.
The test name is misleading. It can be "testDumpWithTableDirMissing".
| run("DROP TABLE " + dbName + ".normal", driver); | ||
| run("drop database " + dbName, true, driver); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add a test case for one of the partition directory is missing.
| @@ -1097,6 +1100,8 @@ public Boolean apply(@Nullable CallerArguments args) { | |||
|
|
|||
| // Retry with different dump should fail. | |||
| replica.loadFailure(replicatedDbName, tuple2.dumpLocation); | |||
| CommandProcessorResponse ret = replica.runCommand("REPL LOAD " + replicatedDbName + " FROM '" + tuple2.dumpLocation + "'"); | |||
There was a problem hiding this comment.
Shall remove the duplicate verification step of REPL LOAD command with replica.loadFailure.
| @@ -156,6 +159,10 @@ private void exportFilesAsList() throws SemanticException, IOException, LoginExc | |||
| } | |||
| done = true; | |||
| } catch (IOException e) { | |||
| if (e instanceof FileNotFoundException) { | |||
| logger.error("exporting data files in dir : " + dataPathList + " to " + exportRootDataDir + " failed"); | |||
| throw new FileNotFoundException(FILE_NOT_FOUND.format(e.getMessage())); | |||
There was a problem hiding this comment.
For same use-case with Acid tables where table dir is missing throws IOException from Utils.getDataPathList instead of FileNotFoundException. Also, add a test case to validate this.
There was a problem hiding this comment.
hadoop team has confirmed that they throw file not found for all cases where file/dir is missing.
|
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. |
No description provided.