rdblue commented on a change in pull request #144: Delete temporary metadata
file when rename fails.
URL: https://github.com/apache/incubator-iceberg/pull/144#discussion_r273129600
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java
##########
@@ -293,4 +309,59 @@ public void testMergeAppend() throws Exception {
Assert.assertEquals("Current snapshot should contain 1 merged manifest",
1, metadata.currentSnapshot().manifests().size());
}
+
+ @Test
+ public void testRenameFailures() throws Exception {
+ Assert.assertTrue("Should create v1 metadata",
+ version(1).exists() && version(1).isFile());
+ Assert.assertFalse("Should not create v2 or newer versions",
+ version(2).exists());
+ assertTrue(table instanceof BaseTable);
+ final BaseTable bt = (BaseTable) table;
+ // use v1 metafile as the test commit destination
+ final TableMetadata meta1 = bt.operations().current();
+
+ // create v2 metafile as base
+ table.updateSchema()
+ .addColumn("n", Types.IntegerType.get())
+ .commit();
+ Assert.assertTrue("Should create v2 for the update",
+ version(2).exists() && version(2).isFile());
+ Assert.assertEquals("Should write the current version to the hint file",
+ 2, readVersionHint());
+
+ // mock / spy the classes for testing
+ final TableOperations tops = bt.operations();
+ assertTrue(tops instanceof HadoopTableOperations);
+ final HadoopTableOperations htops = (HadoopTableOperations) tops;
+ final HadoopTableOperations spyOps = Mockito.spy(htops);
+
+ // mock for rename returning false, verify tempMetadataFile is removed
+ final FileSystem mockFS = mock(FileSystem.class);
+ when(mockFS.exists(any())).thenReturn(false);
+ when(mockFS.rename(any(), any())).thenReturn(false);
+ doReturn(mockFS).when(spyOps).getFS(any(), any());
+
+ verifyTempMetaFileGone(meta1, spyOps);
+
+ // mock for rename throwing, verify tempMetadataFile is removed
+ when(mockFS.rename(any(), any())).thenThrow(new IOException("test
injected"));
+ verifyTempMetaFileGone(meta1, spyOps);
+ }
+
+ /**
+ * Verifies that there is no temporary metadata.json file left on rename
failures.
+ */
+ private void verifyTempMetaFileGone(TableMetadata meta1,
HadoopTableOperations spyOps) {
+ try {
+ spyOps.commit(spyOps.current(), meta1);
+ fail("Commit should fail due to mock file system");
+ } catch (CommitFailedException expected) {
+ LOG.info("Caught expected exception", expected);
+ }
+
+ final String[] children = metadataDir.list((dir, name) ->
name.endsWith(".metadata.json"));
Review comment:
There are helper methods in `HadoopTablesTestBase`, like
`listManifestFiles`. I'd like to see this moved to that class as
`listMetadataJsonFiles`. That way we can use it later more easily.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
