[MINOR] Fix usages of orElse #10435

the-other-tim-brown · 2024-01-01T17:01:24Z

Change Logs

There are places in the codebase where the code uses orElse when it should be orElseGet to avoid running some computation or creation of unnecessary objects.

Impact

Reduce amount of objects created and computation done

Risk level (write none, low medium or high below)

none

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change

The config description must be updated if new configs are added or the default value of the configs are changed
Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
ticket number here and follow the instruction to make
changes to the website.

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

…essary

danny0405 · 2024-01-02T01:13:39Z

...nt/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java

@@ -1016,7 +1016,7 @@ private List<String> getInstantsToRollbackForLazyCleanPolicy(HoodieTableMetaClie
  @Deprecated
  public boolean rollback(final String commitInstantTime, Option<HoodiePendingRollbackInfo> pendingRollbackInfo, boolean skipLocking) throws HoodieRollbackException {
    final String rollbackInstantTime = pendingRollbackInfo.map(entry -> entry.getRollbackInstant().getTimestamp())
-        .orElse(createNewInstantTime(!skipLocking));
+        .orElseGet(() -> createNewInstantTime(!skipLocking));
    return rollback(commitInstantTime, pendingRollbackInfo, rollbackInstantTime, skipLocking);


Okay, so #orElseGet is always preferrable than #orElse.

In general, you do not want to execute methods or create objects you will not use. Therefore you can use orElse when returning a constant but otherwise you should avoid it.

Good catch!

yihua · 2024-01-03T07:20:49Z

...nt/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java

@@ -1016,7 +1016,7 @@ private List<String> getInstantsToRollbackForLazyCleanPolicy(HoodieTableMetaClie
  @Deprecated
  public boolean rollback(final String commitInstantTime, Option<HoodiePendingRollbackInfo> pendingRollbackInfo, boolean skipLocking) throws HoodieRollbackException {
    final String rollbackInstantTime = pendingRollbackInfo.map(entry -> entry.getRollbackInstant().getTimestamp())
-        .orElse(createNewInstantTime(!skipLocking));
+        .orElseGet(() -> createNewInstantTime(!skipLocking));
    return rollback(commitInstantTime, pendingRollbackInfo, rollbackInstantTime, skipLocking);


Good catch!

yihua · 2024-01-03T07:22:54Z

hudi-client/hudi-spark-client/src/main/scala/org/apache/hudi/HoodieSparkUtils.scala

@@ -107,23 +107,19 @@ object HoodieSparkUtils extends SparkAdapterSupport with SparkVersionsSupport wi
    //       injecting [[SQLConf]], which by default isn't propagated by Spark to the executor(s).
    //       [[SQLConf]] is required by [[AvroSerializer]]
    injectSQLConf(df.queryExecution.toRdd.mapPartitions { rows =>
-      if (rows.isEmpty) {
-        Iterator.empty


Does removal of this provide any benefit?

Yes, this will trigger the dag to see if it is in fact returning an empty set of rows. You can see this on the spark UI when running your jobs.

I was looking at the wrong place. This is the spot that needed to be updated: https://github.com/apache/hudi/pull/10435/files#diff-21ccee08cebace9de801e104f356bf4333c017d5d5c0cac4e3de63c7861f3c13R100

yihua · 2024-01-03T07:25:07Z

hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/StreamSync.java

    InputBatch inputBatch = readFromSource(instantTime, metaClient);
+    LOG.error("Time to read from source : " + (System.currentTimeMillis() - startInput));


Use HoodieTimer to track execution time?

This was from my own debugging of slow tests. I will revert

yihua · 2024-01-03T07:25:19Z

hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/StreamSync.java

@@ -448,7 +450,9 @@ public Pair<Option<String>, JavaRDD<WriteStatus>> syncOnce() throws IOException
        }
      }

+      long startWrite = System.currentTimeMillis();


Similar here on using HoodieTimer and below.

hudi-bot · 2024-01-06T21:16:17Z

CI report:

9674bfb Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/StreamSync.java

yihua

LGTM

fix usage of orElse to use orElseGet throughout the codebase when nec…

072ac26

…essary

the-other-tim-brown changed the title ~~fix usage of orElse to use orElseGet throughout the codebase when nec…~~ [MINOR] Fix usage of orElse Jan 1, 2024

the-other-tim-brown marked this pull request as ready for review January 1, 2024 17:15

the-other-tim-brown changed the title ~~[MINOR] Fix usage of orElse~~ [MINOR] Fix usages of orElse Jan 1, 2024

danny0405 reviewed Jan 2, 2024

View reviewed changes

danny0405 approved these changes Jan 2, 2024

View reviewed changes

remove note

402d1eb

yihua reviewed Jan 3, 2024

View reviewed changes

the-other-tim-brown added 4 commits January 3, 2024 07:33

remove debug code

ca74f15

Merge remote-tracking branch 'origin/master' into fix-or-else-usage

4f9744e

remove debug line

2684527

remove is empty check

9674bfb

yihua reviewed Jan 10, 2024

View reviewed changes

hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/StreamSync.java Outdated Show resolved Hide resolved

clean up debugging code

cc0c686

yihua approved these changes Jan 10, 2024

View reviewed changes

yihua merged commit 57a0846 into apache:master Jan 10, 2024
25 of 31 checks passed

the-other-tim-brown deleted the fix-or-else-usage branch January 10, 2024 22:32

VitoMakarevich pushed a commit to VitoMakarevich/hudi that referenced this pull request Jan 13, 2024

[MINOR] Fix usages of orElse (apache#10435)

e66dc1e

VitoMakarevich pushed a commit to VitoMakarevich/hudi that referenced this pull request Jan 13, 2024

[MINOR] Fix usages of orElse (apache#10435)

f2acb80

yihua pushed a commit that referenced this pull request Feb 27, 2024

[MINOR] Fix usages of orElse (#10435)

26df317

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MINOR] Fix usages of orElse #10435

[MINOR] Fix usages of orElse #10435

the-other-tim-brown commented Jan 1, 2024 •

edited

Loading

danny0405 Jan 2, 2024

the-other-tim-brown Jan 2, 2024

yihua Jan 3, 2024

yihua Jan 3, 2024

yihua Jan 3, 2024

the-other-tim-brown Jan 3, 2024

the-other-tim-brown Jan 6, 2024

yihua Jan 10, 2024

yihua Jan 3, 2024

the-other-tim-brown Jan 3, 2024

yihua Jan 3, 2024

hudi-bot commented Jan 6, 2024

yihua left a comment

		InputBatch inputBatch = readFromSource(instantTime, metaClient);
		LOG.error("Time to read from source : " + (System.currentTimeMillis() - startInput));

[MINOR] Fix usages of orElse #10435

[MINOR] Fix usages of orElse #10435

Conversation

the-other-tim-brown commented Jan 1, 2024 • edited Loading

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudi-bot commented Jan 6, 2024

CI report:

yihua left a comment

Choose a reason for hiding this comment

the-other-tim-brown commented Jan 1, 2024 •

edited

Loading