[HUDI-2395] Rewrite metadata tests using HoodieTestTable by nsivabalan · Pull Request #3595 · apache/hudi

nsivabalan · 2021-09-03T15:10:23Z

What is the purpose of the pull request

Adding tests to Metadata table based on HoodieTestTable. Objective is to make the tests lean and consistent. Especially contents of data files does not matter for metadata, we have an opportunity to make it simpler.

Brief change log

Added few building blocks to HoodieTestTable to assist in this testing. Especially metadata sync relies on commitMetadata and hence all such supporting blocks are added to HoodieTestTable.
Added tests covering bootstrap, regular writes, clean, compaction, rollback, inflight operation mid timeline, etc.
Tests based on HoodieTestTable tests just the metadata table and hence sync has to be called explicitly simulating actual sequence of sync calls. So, for now, leaving existing tests in TestHoodieBackedMetadata as is. Will think through more on this and come up with a strategy on how to cover those by tests.
As of now, focussed on COW. Yet to add log files related tests to MOR.
Will be adding more tests to TestHoodieBackedMetadata in the next few days. And eventual goal is to remove all SparkRddWriteClient based tests for metadata.

Verify this pull request

Change itself is just around tests.

Added tests to TestHoodieBackedMetadata

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

hudi-bot · 2021-09-03T15:14:12Z

CI report:

37a2c3e Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run travis re-run the last Travis build
@hudi-bot run azure re-run the last Azure build

nsivabalan · 2021-09-03T15:22:59Z

...i-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java

Note to Reviewer: This is an almost replica of existing ValidateMetadata except that this uses HoodieTestTable as source table for validation. As mentioned in the description, will be adding more tests and eventually will remove direct SparkRDDClient based tests.

vinothchandar

I was hoping the test shrinks in size, not expand? do you plan to do another pass to remove the redundant tests? any improvments in runtime.

In general, I want to make sure the new methods added to HoodieTestTable are in line with its design. @xushiyan any comments on that?

vinothchandar · 2021-09-03T15:54:50Z

hudi-common/src/test/java/org/apache/hudi/common/testutils/FileCreateUtils.java

this does not belong here? in FileCreateUtils? its not really creating anything

there exists some functions too like getXXX and deleteXXX. Maybe it's time to rename this to FileCRUDUtils?

vinothchandar · 2021-09-03T15:57:06Z

hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestTable.java

can this be less verbose, like calling .compare on the getModificationTime()?

vinothchandar · 2021-09-03T15:57:57Z

hudi-common/src/test/java/org/apache/hudi/common/testutils/PartitionFileInfoMap.java

this map needs to be made nicer to read?

xushiyan · 2021-09-07T06:49:44Z

In general, I want to make sure the new methods added to HoodieTestTable are in line with its design. @xushiyan any comments on that?

@vinothchandar @nsivabalan At the beginning we were thinking make HoodieTestTable provide concise APIs doing basic table operations with empty files and HoodieWriteableTestTable writing actual data. FileCreateUtils is used by HoodieTestTable at lower level interfacing with the actual files. When more and more APIs are introduced, we have to re-write many logics and re-implement action-to-file-change translations. This resulted in the testutils having cumbersome code and learning hurdle, also error-prone. I felt some re-design is needed.

Some thoughts on the re-design: developers are familiar with HoodieXXXClient so we would need HoodieTestTable make use of some client with similar public APIs, say HoodieTestTableClient.

HoodieTestTable owns a HoodieTestTableClient and exposes the client's APIs for dev to prep their own data
HoodieTestTableClient should manipulate timeline and metadata as normal
For UTs don't need real data, the client writes out empty log and base files.
For FTs, the test table and client should be aware of the EngineContext set by XXXFunctionalTestHarness and act like the actual clients
Eventually dev only learn APIs provided by HoodieTestTableClient to prep their tests. It can include high-level APIs like .write100RecordsIn3Partitions() as such.

xushiyan · 2021-09-10T16:12:07Z

...i-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java

+    tableType = HoodieTableType.COPY_ON_WRITE;
+    init(tableType);


ideally all test cases should parameterize with table type

@ParameterizedTest @EnumSource(HoodieTableType.class)

xushiyan · 2021-09-10T16:25:49Z

...i-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java

+    testBootstrap(testTable,true);
+  }
+
+  private void testBootstrap(HoodieTestTable testTable, boolean addRollback) throws Exception {


i've seen this pattern in many classes: create a private method doing all the test steps with a variable control different scenarios while different testing methods invoke it with the variable. We should start avoiding this, for reasons

control flow is an anti-pattern in test code. Each testcase just follows a simple flow: prep -> execute -> verify. Any varying part can be moved to a different test method to explicitly show a different scenario

I can see the use of control flow is mainly to reuse some code in the original flow. It's a sign that the original flow's code itself is not concise enough to be repeated. I think repeating some code across testcase is acceptable and even preferred: testcases should be isolated and people wants to read the flow as is without jumping back and forth btw methods. Repeating concise test prep and verification logic makes the scenario more readable and manageable in 1 place. This requires the test utils classes properly refactored and doing heavy liftings.

xushiyan · 2021-09-10T16:29:06Z

...i-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java

+  private void testBootstrap(HoodieTestTable testTable, boolean addRollback) throws Exception {
+
+    // bootstrap w/ 3 or 5 commits
+    testTable.doWriteOperation(testTable, "001", WriteOperationType.INSERT, Arrays.asList("p1", "p2"), Arrays.asList("p1", "p2"),


try making use of varargs instead of List for test util APIs. varargs gives more flexibility and does not require caller to build a list (less code)

xushiyan · 2021-09-10T17:06:07Z

...i-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java

+  /**
+   * 1. Enable metadata to sync and validate.
+   * 2. Disable metadata and add few writes to table.
+   * 3. Enable back again to sync and validate.
+   * @throws Exception
+   */


@throws Exception looks redundant here. most of the time we just let exception throw and investigate the failure.

if test logic is encapsulate in well-design util APIs, we may not need extra javadoc to explain the flow. Some inline comments might still be helpful but ideally code itself should be able to explain it pretty well

xushiyan · 2021-09-10T17:09:44Z

...i-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java

+        assertEquals(fsStatuses.length, partitionToFilesMap.get(basePath + "/" + partition).length);
+
+        // File sizes should be valid
+        Arrays.stream(metaStatuses).forEach(s -> assertTrue(s.getLen() > 0));


we should prefer for-loop over lambda in test code when there is exception to avoid try-catch block. Just declare exception all the way up we can anyway capture it when test failed.

xushiyan · 2021-09-10T17:15:20Z

hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestTable.java

  }

+  public HoodieCommitMetadata createCommitMetadata(WriteOperationType operationType, String commitTime,
+                                                   Map<String, List<Pair<String, Integer>>> partitionToFileIdMap) {


should try encapsulate data structure like partitionToFileIdMap within HoodieTestState and make it invisible to users. It's not easy to grasp and keep recalling what info is kept in the Map. And more friction of using it in an API

xushiyan · 2021-09-10T17:17:14Z

hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestTable.java

+    return testTable.addCompaction(commitTime, commitMetadata);
+  }
+
+  public Pair<HoodieCommitMetadata, PartitionFileInfoMap> doWriteOperation(HoodieTestTable testTable, String commitTime, WriteOperationType operationType,


this is an instance method, it does not need user to pass in a testTable. Unless you want this to be static?

xushiyan · 2021-09-10T17:18:24Z

hudi-common/src/test/java/org/apache/hudi/common/testutils/PartitionDeleteFileList.java

+import java.util.List;
+import java.util.Map;
+
+public class PartitionDeleteFileList {


as discussed, we can start creating a HoodieTestState and encapsulate it there.

xushiyan · 2021-09-10T17:18:34Z

hudi-common/src/test/java/org/apache/hudi/common/testutils/PartitionFileInfoMap.java

+import java.util.Map;
+import java.util.UUID;
+
+public class PartitionFileInfoMap {


xushiyan · 2021-09-10T17:19:01Z

hudi-common/src/test/java/org/apache/hudi/common/testutils/PartitionFileInfoMap.java

+  public Map<String, List<Pair<String, Integer>>> getPartitionToFileIdMap(String commitTime) {
+    return this.partitionToFileIdMap.get(commitTime);
+  }
+}


should fix the IDE setting to auto fix EOL problem

nsivabalan · 2021-09-23T19:43:36Z

Closing in favor of #3695

nsivabalan force-pushed the metadataTestsRewrite branch from 0183d68 to 031fe67 Compare September 3, 2021 15:20

nsivabalan commented Sep 3, 2021

View reviewed changes

nsivabalan marked this pull request as ready for review September 3, 2021 15:24

vinothchandar reviewed Sep 3, 2021

View reviewed changes

[HUDI-2395] Rewrite metadata tests using HoodieTestTable

37a2c3e

nsivabalan force-pushed the metadataTestsRewrite branch from 031fe67 to 37a2c3e Compare September 3, 2021 16:22

vinothchandar self-assigned this Sep 7, 2021

xushiyan reviewed Sep 10, 2021

View reviewed changes

codope mentioned this pull request Sep 21, 2021

[HUDI-2395] Metadata tests rewrite #3695

Merged

5 tasks

nsivabalan closed this Sep 23, 2021

Conversation

nsivabalan commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

hudi-bot commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xushiyan commented Sep 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsivabalan commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nsivabalan commented Sep 3, 2021 •

edited

Loading

hudi-bot commented Sep 3, 2021 •

edited

Loading