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

doc: added deletion overview with very high level chart. #288 #295

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

panta-123
Copy link

Added the deletion overveiw in different chapter.

Few things I am not clear.

  1. When setting TOMBSTONE by cleaner and undertaker , what is the value it sets. I look through the code and not clear to me.

  2. The chart are very crude and may need some imporvement suggestion needed here.

@panta-123
Copy link
Author

All the comments has been resolved !!

Copy link
Member

@bari12 bari12 left a comment

Choose a reason for hiding this comment

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

Just one more change :-)

Copy link
Contributor

@voetberg voetberg left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! 👍

@voetberg
Copy link
Contributor

@bari12 Can you verify your requested changes are completed?

@voetberg voetberg requested a review from bari12 November 14, 2024 14:35
@bari12
Copy link
Member

bari12 commented Nov 20, 2024

It's good. Thanks @panta-123 and @voetberg
You need to add the file to the sidebars.json though, otherwise it won't show up in the doc

Copy link
Contributor

@rdimaio rdimaio left a comment

Choose a reason for hiding this comment

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

In case future changes need to be done, it would be great to have the source code for the flowcharts as part of the documentation repo. Maybe we could use Mermaid.js to handle this?

Copy link
Contributor

Choose a reason for hiding this comment

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

"get list of all replica with tombstone" -> "get list of all replicas with tombstone"

Copy link
Contributor

Choose a reason for hiding this comment

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

There are two identical "get list of all replicas with tombstone" blocks; the chart could be simplified so that there is only one block, to which you can either get via "Is greedy? YES" or "needed_free_space >= 0"

Copy link
Author

Choose a reason for hiding this comment

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

There is another step for needed_free_space so that will complicate things if pointrd to single blocks.

Copy link
Author

Choose a reason for hiding this comment

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

@rdimaio , The mermaid.js lets create another issue and handle all the chart there.

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