Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MINOR] Merge logs into check instant file of HoodieActiveTimeline.transitionPendingState #10392

Merged
merged 2 commits into from
Dec 23, 2023

Conversation

zhuanshenbsj1
Copy link
Contributor

Change Logs

Before modification:

image

Make the Exception information more clearer .

Impact

Describe any public API or user-facing feature change or any performance impact.

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

If medium or high, explain what verification was done to mitigate the risks.

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

@@ -695,8 +695,8 @@ protected void transitionPendingState(HoodieInstant fromInstant, HoodieInstant t
}
} else {
// Ensures old state exists in timeline
LOG.info("Checking for file exists ?" + getInstantFileNamePath(fromInstantFileName));
ValidationUtils.checkArgument(metaClient.getFs().exists(getInstantFileNamePath(fromInstantFileName)));
ValidationUtils.checkArgument(metaClient.getFs().exists(getInstantFileNamePath(fromInstantFileName)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhuanshenbsj1 : Looks like you are removing log message while the title says adding log. what is the reason for this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like he merged the log msg into the exception msg, make sense somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception information here is not clear. I want to move the log to exception info, to clearly inform the user of the reason for the exception.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sound good. Thanks both for the clarification

…e/HoodieActiveTimeline.java

Co-authored-by: Danny Chan <yuzhao.cyz@gmail.com>
@danny0405 danny0405 changed the title [MINOR] add log for HoodieActiveTimeline.transitionPendingState to check instant file [MINOR] Add log for HoodieActiveTimeline.transitionPendingState to check instant file Dec 22, 2023
@zhuanshenbsj1 zhuanshenbsj1 changed the title [MINOR] Add log for HoodieActiveTimeline.transitionPendingState to check instant file [MINOR] Merge logs into check instant file of HoodieActiveTimeline.transitionPendingState Dec 22, 2023
@hudi-bot
Copy link

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@danny0405 danny0405 merged commit c2da8aa into apache:master Dec 23, 2023
29 of 33 checks passed
yihua pushed a commit that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants