[HUDI-3469] Refactor `HoodieTestDataGenerator` to provide for reproducible Builds #4866

alexeykudinkin · 2022-02-21T21:42:49Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contribute/how-to-contribute before opening a pull request.

What is the purpose of the pull request

Refactoring HoodieTestDataGenerator to provide for reproducible Builds: currently, HoodieTestDataGenerator relies on static state which make its state shared across all of the tests making data generation dependent on the order of execution.

Instead we should properly abstract HoodieTestDataGenerator to hold all of the state w/in individual instances so that individual Tests can:

Create they own isolated instance (which won't be affected by other Tests)
Pass "seed" value to DataGenerator to init its PRNG w/ it, so that it always produces the same (pseudo-)random sequence (for a given seed)
Be certain that all of the data produced by DataGenerator will be 100% reproducible w/ the same seed (meaning that all of the DataGenerator operations w/in it only rely on such internal PRNG and don't rely on any external sources, such as UUID.randomUUID(), System.currentTimeMillis(), etc)

Brief change log

See above

Verify this pull request

This pull request is already covered by existing tests, such as (please describe tests).

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.

yihua

Overall LGTM. Thanks for the improvement! I left a few moments.

yihua · 2022-02-21T21:57:58Z

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

+    bytes[6] &= 0x0f;
+    bytes[6] |= 0x40;
+    bytes[8] &= 0x3f;
+    bytes[8] |= 0x80;


what's the reason for having this logic? trying to bound the value?

Some standard v4 bits clearing/setting

maybe this method deserves some usage guide in javadoc?

yihua · 2022-02-21T21:58:30Z

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

  public static final Schema FLATTENED_AVRO_SCHEMA = new Schema.Parser().parse(TRIP_FLATTENED_SCHEMA);

-  private static final Random RAND = new Random(46474747);
+  private final Random r;


nit: rename to rand?

:-)

Why do you think rand is better than r?

I actually tried to search the variable and see if anything is missed. r gives me hard time :) Besides, I usually use rand and try to avoid single character variable. More like a style thing, not strong opinion.

Fair enough

yihua · 2022-02-21T22:01:00Z

hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestCleansCommand.java

    String fileId2 = UUID.randomUUID().toString();
    FileSystem fs = FSUtils.getFs(basePath(), hadoopConf());
-    HoodieTestDataGenerator.writePartitionMetadata(fs, HoodieTestDataGenerator.DEFAULT_PARTITION_PATHS, tablePath);
+    HoodieTestDataGenerator.writePartitionMetadataDeprecated(fs, HoodieTestDataGenerator.DEFAULT_PARTITION_PATHS, tablePath);


Are these going to be cleaned up in a separate PR?

Nah, we can let those die down naturally

yihua · 2022-02-21T22:01:55Z

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

  public static final TypeDescription ORC_TRIP_SCHEMA = AvroOrcUtils.createOrcSchema(new Schema.Parser().parse(TRIP_SCHEMA));
  public static final Schema FLATTENED_AVRO_SCHEMA = new Schema.Parser().parse(TRIP_FLATTENED_SCHEMA);

-  private static final Random RAND = new Random(46474747);


Looks like this always generates the same seq of "random" numbers before?

yihua · 2022-02-21T22:05:13Z

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

+  }
+
+  public HoodieTestDataGenerator(long seed, String[] partitionPaths, Map<Integer, KeyPartition> keyPartitionMap) {
+    this.r = new Random(seed);


Should the seed be printed in logs here, so that if CI runs fail, the seed can be used for reproducing locally?

Did you fix this somewhere else?

yihua · 2022-02-21T22:10:15Z

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

-        () -> partitionPaths[RAND.nextInt(partitionPaths.length)],
-        () -> UUID.randomUUID().toString());
+        () -> partitionPaths[r.nextInt(partitionPaths.length)],
+        () -> genPseudoRandomUUID(r).toString());


Should this be fixed as well:

public List<HoodieRecord> generateInsertsForPartition(String instantTime, Integer n, String partition) { return generateInsertsStream(instantTime, n, false, TRIP_EXAMPLE_SCHEMA, false, () -> partition, () -> UUID.randomUUID().toString()).collect(Collectors.toList()); }

Good catch! Weirdly my text search skipped through these

yihua · 2022-02-21T22:11:25Z

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

    for (int i = 0; i < limit; i++) {
-      String partitionPath = partitionPaths[RAND.nextInt(partitionPaths.length)];
+      String partitionPath = partitionPaths[r.nextInt(partitionPaths.length)];
      HoodieKey key = new HoodieKey(UUID.randomUUID().toString(), partitionPath);


Same here for UUID.randomUUID().

yihua · 2022-02-21T22:13:20Z

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

  public List<GenericRecord> generateGenericRecords(int numRecords) {
    List<GenericRecord> list = new ArrayList<>();
    IntStream.range(0, numRecords).forEach(i -> {
      list.add(generateGenericRecord(UUID.randomUUID().toString(), "0", UUID.randomUUID().toString(), UUID.randomUUID()


same here regarding UUID.randomUUID() if full reproducibility is the goal.

yihua · 2022-02-21T22:17:51Z

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

      rec.put("nation", ByteBuffer.wrap(bytes));
-      long currentTimeMillis = System.currentTimeMillis();
-      Date date = new Date(currentTimeMillis);
+      long randomMillis = genRandomTimeMillis(r);


One nit for thought. Previously, each time generateGenericRecord() is called, the timestamp monotonically increases. If some tests depend on this assumption, the new time millis random gen should also honor that in a way, e.g., start with a random millis, and increment it by another positive random millis for each subsequent call, to guarantee the order.

I don't think this is an assumption that tests should be able to make. At the end of the day there's no such contract provided by the Generator, and tests should not depend on such implementation detail.

xushiyan

LGTM

xushiyan · 2022-02-25T01:57:48Z

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

-      long currentTimeMillis = System.currentTimeMillis();
-      Date date = new Date(currentTimeMillis);
+      long randomMillis = genRandomTimeMillis(r);
+      Date date = new Date(randomMillis);


optional: while you're on this, it may worth migrating away from Date to java.time APIs, for standardizing datetime usage in the codebase and be thread-safe where applicable.

Yeah, no problem. Which one are you referring to? There's no such thing as DateTime in java.time

It's probably LocalDate we need

xushiyan · 2022-02-25T02:08:20Z

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

+    bytes[6] &= 0x0f;
+    bytes[6] |= 0x40;
+    bytes[8] &= 0x3f;
+    bytes[8] |= 0x80;


maybe this method deserves some usage guide in javadoc?

…ible

alexeykudinkin · 2022-03-02T03:04:51Z

@hudi-bot run azure

alexeykudinkin · 2022-03-02T04:49:19Z

@hudi-bot run azure

hudi-bot · 2022-03-02T05:56:24Z

CI report:

180ea55 UNKNOWN
ff1ccbd UNKNOWN
95c0c7b UNKNOWN
ac2ffe0 Azure: FAILURE Azure: FAILURE Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

yihua

LGTM

…cible Builds (apache#4866)

alexeykudinkin force-pushed the ak/tst-dgen-ref branch from c104012 to 062da23 Compare February 21, 2022 21:43

alexeykudinkin changed the title ~~[HUDI-3469] Refactor HoodieTestDataGenerator to provide for reproducible Builds~~ [HUDI-3469] Refactor HoodieTestDataGenerator to provide for reproducible Builds Feb 21, 2022

yihua reviewed Feb 21, 2022

View reviewed changes

yihua self-assigned this Feb 21, 2022

alexeykudinkin force-pushed the ak/tst-dgen-ref branch from f1eafce to e6627d1 Compare February 24, 2022 01:07

xushiyan approved these changes Feb 25, 2022

View reviewed changes

alexeykudinkin force-pushed the ak/tst-dgen-ref branch from 2c85afe to ff1ccbd Compare March 1, 2022 03:21

Alexey Kudinkin added 11 commits February 28, 2022 19:27

Refactored HoodieTestDataGenerator to make data generation reproduc…

af847d2

…ible

lint

7be2b5f

Fixing missing refs to UUID.randomUUID

99776a8

Restricted timestamp generation to anchor date +-3 days

6c71cb8

Cleaning up: made key-gen field final

962b8cf

Fixing tests

002516d

Tidying up

1e885b1

Tidying up

6fda7bb

lint

6b2447f

Tidying up

d8edb1c

Fixing compilation

972b494

alexeykudinkin force-pushed the ak/tst-dgen-ref branch from 95c0c7b to 972b494 Compare March 1, 2022 03:30

Log seed

ac2ffe0

yihua approved these changes Mar 2, 2022

View reviewed changes

yihua merged commit 85f47b5 into apache:master Mar 2, 2022

vingov pushed a commit to vingov/hudi that referenced this pull request Apr 3, 2022

[HUDI-3469] Refactor HoodieTestDataGenerator to provide for reprodu…

89c89cb

…cible Builds (apache#4866)

stayrascal pushed a commit to stayrascal/hudi that referenced this pull request Apr 12, 2022

[HUDI-3469] Refactor HoodieTestDataGenerator to provide for reprodu…

466a633

…cible Builds (apache#4866)

[HUDI-3469] Refactor HoodieTestDataGenerator to provide for reproducible Builds #4866

[HUDI-3469] Refactor HoodieTestDataGenerator to provide for reproducible Builds #4866

Uh oh!

Conversation

alexeykudinkin commented Feb 21, 2022

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

yihua 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!

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!

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 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!

alexeykudinkin commented Mar 2, 2022

Uh oh!

alexeykudinkin commented Mar 2, 2022

Uh oh!

hudi-bot commented Mar 2, 2022

CI report:

Uh oh!

yihua left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

[HUDI-3469] Refactor `HoodieTestDataGenerator` to provide for reproducible Builds #4866

[HUDI-3469] Refactor `HoodieTestDataGenerator` to provide for reproducible Builds #4866