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

https://issues.redhat.com/browse/ACM-13324 #7246

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

jc-berger
Copy link
Collaborator

@jc-berger jc-berger requested a review from birsanv November 18, 2024 19:23
@jc-berger jc-berger self-assigned this Nov 18, 2024
@jc-berger jc-berger removed the request for review from birsanv November 18, 2024 19:39
Copy link

@sahare sahare left a comment

Choose a reason for hiding this comment

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

Looks good to me, Thanks!

[#prevent-backup-collision]
== Preventing backup collisions

To prevent and report backup collisions, a `BackupCollision` state exists for the `BackupSchedule.cluster.open-cluster-management.io` resource. The controller checks regularly if the latest backup in the storage location has been generated from the current hub cluster. If not, a different hub cluster has recently written backup data to the storage location, indicating that the hub cluster is colliding with a different hub cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent and report backup collisions, a BackupCollision state exists for the BackupSchedule.cluster.open-cluster-management.io resource.

This line is confusing, I think the word exists here and To prevent.....

I think rearrage this just to say X does Y??


To prevent and report backup collisions, a `BackupCollision` state exists for the `BackupSchedule.cluster.open-cluster-management.io` resource. The controller checks regularly if the latest backup in the storage location has been generated from the current hub cluster. If not, a different hub cluster has recently written backup data to the storage location, indicating that the hub cluster is colliding with a different hub cluster.

In this case, the current hub cluster `BackupSchedule.cluster.open-cluster-management.io` resource status is set to `BackupCollision` and the `Schedule.velero.io` resources created by this resource are deleted to prevent data corruption. The `BackupCollision` is reported by the backup policy. The administrator verifies which hub cluster writes to the storage location, before removing the `BackupSchedule.cluster.open-cluster-management.io` resource from the invalid hub cluster and creating a new `BackupSchedule.cluster.open-cluster-management.io` resource on the valid primary hub cluster, to resume the backup.
Copy link
Contributor

Choose a reason for hiding this comment

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

In all of the tech writing, please look at words like "in this case" and look to eliminate it as not concise or extra words. I also think this is another example of a wall of text and needs to be reduced/reorganized a bit. I will leave that to you since we have gone through an exercise or two of that.

Copy link
Contributor

@swopebe swopebe left a comment

Choose a reason for hiding this comment

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

see comments

@openshift-ci openshift-ci bot removed the lgtm label Nov 20, 2024
Copy link

@sahare sahare left a comment

Choose a reason for hiding this comment

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

Looks good, Thank you!

@openshift-ci openshift-ci bot added the lgtm label Nov 26, 2024

To prevent and report backup collisions, use the `BackupCollision` state in the `BackupSchedule.cluster.open-cluster-management.io` resource. The controller checks regularly if the latest backup in the storage location has been generated from the current hub cluster. If not, a different hub cluster has recently written backup data to the storage location, indicating that the hub cluster is colliding with a different hub cluster.

In the backup collision scenario, the current hub cluster `BackupSchedule.cluster.open-cluster-management.io` resource status is set to `BackupCollision`. To prevent data corruption, this resource deletes the `Schedule.velero.io` resources. The backup policy reports the `BackupCollision`. In this same scenario, the administrator verifies which hub cluster writes to the storage location. The administrator does this verification before removing the `BackupSchedule.cluster.open-cluster-management.io` resource from the invalid hub cluster and creating a new `BackupSchedule.cluster.open-cluster-management.io` resource on the valid primary hub cluster, resuming the backup.
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph is a little long, I'd recommend breaking it up

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@oafischer please see Dev feedback asking to keep it as one paragraph.

I'm happy to tighten it sentence by sentence, but I was requested to keep it as one paragraph as a way to maintain user focus and understanding.

Screenshot 2024-12-02 at 10 02 54 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest separating it anyway, we've gotten customer feedback about reducing walls of text. I'll leave the final decision up to you but I'd argue that the customer feedback takes priority. We can look at connecting both paragraphs better with other writing methods, if necessary.

Copy link
Contributor

@swopebe swopebe Dec 2, 2024

Choose a reason for hiding this comment

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

I agree with @oafischer and we have feedback to "avoid walls of text" that I have shared in our doc meetings. As the writer, we take authority of the UX of the doc and find a way to make the paragraph concise without losing meaning. I can help with another exercise if that will be beneficial. I can tell by looking at it that we can break it up and remove some unnecessary words for better UX. Let me know if you would like to go over this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@swopebe Hi, thanks for you input.

Yes, I'm aware of the "walls of text" that you discussed in meetings. And yes, I'm aware that as the writer, we take authority of the UX. As you'll see in Sahar's comment, her criticism is that understanding was lost and the user would be lost. Your criticism was that the content was long. Therefore, taking ownership of the content, I prioritized the feedback that said the content was unclear and that the situation wasn't adequately described. With that said, I did still implement feedback of yours like when you said to remove the phrase "in this case" and shorten the sentences.

This PR is an example in which I was asked to do two different, contrasting things by the peer reviewers and I think, I found a good middle ground which still addressed your primary concern of shortening the content and fixing the headings and organization.

Also, I agree this paragraph is a little on the long side, especially the last sentence. However, it was 5 sentences and 88 words, neither of which are considered overly long from a documentation standpoint. As the tech writer of this content, the SME told me the situation wasn't described clearly enough. Applying this feedback, I wrote a sort paragraph of less than 100 words which the SME approved, so I actually wouldn't consider this to be an egregious example of "wall of text"

Nevertheless, I made another commit. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the separation you provided makes sense and is much needed.


To prevent and report backup collisions, use the `BackupCollision` state in the `BackupSchedule.cluster.open-cluster-management.io` resource. The controller checks regularly if the latest backup in the storage location has been generated from the current hub cluster. If not, a different hub cluster has recently written backup data to the storage location, indicating that the hub cluster is colliding with a different hub cluster.

In the backup collision scenario, the current hub cluster `BackupSchedule.cluster.open-cluster-management.io` resource status is set to `BackupCollision`. To prevent data corruption, this resource deletes the `Schedule.velero.io` resources. The backup policy reports the `BackupCollision`. In this same scenario, the administrator verifies which hub cluster writes to the storage location. The administrator does this verification before removing the `BackupSchedule.cluster.open-cluster-management.io` resource from the invalid hub cluster and creating a new `BackupSchedule.cluster.open-cluster-management.io` resource on the valid primary hub cluster, resuming the backup.
Copy link
Contributor

@swopebe swopebe Dec 2, 2024

Choose a reason for hiding this comment

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

I agree with @oafischer and we have feedback to "avoid walls of text" that I have shared in our doc meetings. As the writer, we take authority of the UX of the doc and find a way to make the paragraph concise without losing meaning. I can help with another exercise if that will be beneficial. I can tell by looking at it that we can break it up and remove some unnecessary words for better UX. Let me know if you would like to go over this.

@openshift-ci openshift-ci bot removed the lgtm label Dec 2, 2024
@openshift-ci openshift-ci bot added the lgtm label Dec 2, 2024
Copy link

openshift-ci bot commented Dec 2, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jc-berger, sahare, swopebe

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jc-berger jc-berger merged commit 54ae565 into 2.12_stage Dec 2, 2024
1 of 2 checks passed
@jc-berger jc-berger deleted the jcberger-13324-fix-avoiding-topic branch December 2, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants