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

IGNITE-15062 Events for snapshot restore operation. #9240

Merged
merged 3 commits into from
Aug 4, 2021

Conversation

xtern
Copy link
Contributor

@xtern xtern commented Jul 8, 2021

Thank you for submitting the pull request to the Apache Ignite.

In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:

The Contribution Checklist

  • There is a single JIRA ticket related to the pull request.
  • The web-link to the pull request is attached to the JIRA ticket.
  • The JIRA ticket has the Patch Available state.
  • The pull request body describes changes that have been made.
    The description explains WHAT and WHY was made instead of HOW.
  • The pull request title is treated as the final commit message.
    The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
  • A reviewer has been mentioned through the JIRA comments
    (see the Maintainers list)
  • The pull request has been checked by the Teamcity Bot and
    the green visa attached to the JIRA ticket (see TC.Bot: Check PR)

Notes

If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.

@@ -86,6 +87,12 @@
/** Reject operation message. */
private static final String OP_REJECT_MSG = "Cache group restore operation was rejected. ";

/** Snapshot restore operation finish message. */
private static final String OP_FINISHED_MSG = "Successfully restored cache group(s) from the snapshot [reqId=%s]";
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the right word order Subject + Verb + Object: Cache groups have been successfully restored from snapshot.
Do we need a request Id? I'd rather use the cache group names or remove it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These constants were introduced in order to use the same message in logs and events.
This is how the logging looks now, I don't think there is a problem with changing this wording in the log.
But I think we must specify the request ID so that we can find the rest of the events/log-messages/etc related to this one.

private static final String OP_FINISHED_MSG = "Successfully restored cache group(s) from the snapshot [reqId=%s]";

/** Snapshot restore operation failed message. */
private static final String OP_FAILED_MSG = "Failed to restore snapshot cache group [reqId=%s%s]";
Copy link
Contributor

Choose a reason for hiding this comment

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

reqId=%s%s why do we mix a request id and message here?

Copy link
Contributor Author

@xtern xtern Aug 2, 2021

Choose a reason for hiding this comment

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

What kind of problem do you see here?
This is the general message format for the event and a log message. Additional %s adds the error message to the event message (but does not include it in the log message, since we handle errors in the log in a different way).

@Mmuzaf Mmuzaf merged commit 5d8d637 into apache:master Aug 4, 2021
Vladsz83 added a commit to Vladsz83/ignite that referenced this pull request Aug 13, 2021
xintrian pushed a commit to xintrian/ignite that referenced this pull request May 27, 2022
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.

2 participants