[HUDI-3252] Avoid creating empty requestedReplaceCommit in the startCommit method

[dongkelun]

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

(For example: This pull request adds quick-start document.)

Brief change log

(for example:)

• Modify AnnotationLocation checkstyle rule in checkstyle.xml

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

(or)

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

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end.
• Added HoodieClientWriteTest to verify the change.
• Manually verified the change by running a job locally.

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.

[dongkelun]

@xushiyan @nsivabalan hello, can you please take a review?

[xushiyan]

@dongkelun the warn log comes from clustering planning. can you help clarify how would this change avoid the repeated warn logs?

[dongkelun]

@dongkelun the warn log comes from clustering planning. can you help clarify how would this change avoid the repeated warn logs?

Hello, the reason for the warning is that the content of replaceCommitRequestedInstantinstant is empty,There are many places to call this method, such as HoodieSparkTable.create:

    if (refreshTimeline) {
      hoodieSparkTable.getHoodieView().sync();
    }

There are also many places to call HoodieSparkTable.create,Therefore, it is not easy to reduce the warning log. It is better to avoid this warning directly from the source.INSERT_OVERWRITE's commitActionType is REPLACE_COMMIT_ACTION,It creates an empty replaceCommitRequestedInstantinstant in the startCommitWithTime method,We can avoid this warning from the source by changing it to non empty

[nsivabalan]

@codope : Can you review this PR. May be there could be some follow ups in other places where we do create similar empty replace commit instants. I

[codope]

@dongkelun Rather than introduce a behavior change, I would prefer to remove the warn log itself. Is there any reason we still want to keep it?
Also, while you're at it, can you make the method getRequestedReplaceMetadata private? I don't see it being used anywhere other than the same class.

[nsivabalan]

I would vote to remote the log.warn msg only(ClusteringUtils L 93)) using a diff patch which we can pull into 0.10.1. we can continue discussions in this patch as to whats the right fix is.

[nsivabalan]

#4553

[dongkelun]

@dongkelun Rather than introduce a behavior change, I would prefer to remove the warn log itself. Is there any reason we still want to keep it? Also, while you're at it, can you make the method getRequestedReplaceMetadata private? I don't see it being used anywhere other than the same class.

OK, I have made the method getRequestedReplaceMetadata in classes MetadataConversionUtils and ClusteringUtils private

[dongkelun]

@nsivabalan @codope
I found a previous issuse:HUDI-1739 in 'MetadataConversionUtils. getRequestedReplaceMetadata ` is the same as this issuse

#2784

[nsivabalan]

LGTM.

[nsivabalan]

@xushiyan @codope : if you folks don't have any other comments, I can go ahead and land this.

[codope]

Looks good. Just one minor comment.

[xushiyan]

@dongkelun @nsivabalan @codope this PR has pivoted and it's mostly not about warn log any more. Can we file a different ticket tailored to this PR and link to it?

[dongkelun]

@dongkelun @nsivabalan @codope this PR has pivoted and it's mostly not about warn log any more. Can we file a different ticket tailored to this PR and link to it?

I have created a new issue HUDI-3252 and modified the PR link to it. Do you think this description is accurate？

[nsivabalan]

looks good to me. I have closed the previous ticket btw.

[hudi-bot]

CI report:

• 88c372f UNKNOWN
• 1a657de UNKNOWN
• 1903717 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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