[GitHub] [gobblin] meethngala commented on a diff in pull request #3697: [GOBBLIN-1835]Upgrade Iceberg Version from 0.11.1 to 1.2.0

["meethngala (via GitHub)" <gi...@apache.org>]

meethngala commented on code in PR #3697:
URL: https://github.com/apache/gobblin/pull/3697#discussion_r1228556415


##########
gobblin-iceberg/src/test/java/org/apache/gobblin/iceberg/writer/IcebergMetadataWriterTest.java:
##########
@@ -365,12 +364,6 @@ public void testFaultTolerant() throws Exception {
             new LongWatermark(52L))));
     Assert.assertEquals(gobblinMCEWriter.getDatasetErrorMap().size(), 1);
     Assert.assertEquals(gobblinMCEWriter.getDatasetErrorMap().values().iterator().next().size(), 1);
-    Assert.assertEquals(gobblinMCEWriter.getDatasetErrorMap()
-        .get(new File(tmpDir, "testDB/testTopic").getAbsolutePath())
-        .get("hivedb.testTopic").get(0).lowWatermark, 50L);
-    Assert.assertEquals(gobblinMCEWriter.getDatasetErrorMap()
-        .get(new File(tmpDir, "testDB/testTopic").getAbsolutePath())
-        .get("hivedb.testTopic").get(0).highWatermark, 52L);

Review Comment:
   yeah... I was also thinking the same if I could still check for the watermarks. So previously, since a single table was used and we flushed it using `gobblinMCEWriter` (in other unit test cases outside of testing completeness) before attempting `writeEnvelope` and could compare watermarks being unchanged. Now, we use a different table: `testTopicCompleteness` and writer for testing completeness: `gobblinMCEWriterWithCompletness` and the error map only contains the exception that we throw when we attempt to write a record as part of testing the fault tolerance: `Mockito.doThrow(new IOException("Test failure")).when(mockWriter).writeEnvelope(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())`.  I have updated the assert in my latest commit to verify the content of the error map and still achieve the goal of testing fault tolerance after we've successfully flushed the metadata writer. Let me know if my understanding is correct @jack-moseley since I see you're the last person
  who worked on this test case!



-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org