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

Edits to Pgd/5/consistency/crdt/docs 287 PR5928 #6076

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

ebgitelman
Copy link
Contributor

What Changed?

@ebgitelman ebgitelman requested a review from a team as a code owner September 19, 2024 17:33
@@ -138,10 +140,9 @@ SELECT id, counter FROM crdt_delta_example WHERE id = 1;
(1 row)
```

With a regular `integer` column, the result is `2`. But when you update the row with a delta CRDT counter, you start with the OLD row version, make a NEW row version, and send both to the remote node. There, compare them with the version found there (e.g., the LOCAL version). Standard CRDTs merge the NEW and the LOCAL version, while delta CRDTs compare the OLD and NEW versions and apply the delta
With a regular `integer` column, the result is `2`. But when you update the row with a delta CRDT counter, you start with the OLD row version, make a NEW row version, and send both to the remote node. There, compare them with the version found there (for example, the LOCAL version). Standard CRDTs merge the NEW and the LOCAL version. Delta CRDTs compare the OLD and NEW versions and apply the delta
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are the u/c words here being used for emphasis or is this referring to something appears in code? If emphasis, we don't use all caps for this purpose or really any kind of emphasis, and I would lower case them. If referring to something that appears in the code, that's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@djw-m do you happen to know the answer to this question? I am not sure if this is from code or not, as I am not familiar enough with this content.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are being used to label the concepts of NEW, OLD, and LOCAL in reference to rows. Lowercasing them will make this somewhat harder to parse. These aren't code but they do need a labelling emphasis and upper-case is all we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will leave.

@@ -20,22 +20,26 @@ The main disadvantage is that you can't reset this value reliably in an asynchro

!!! Note
Implementing more complicated operation-based types by creating custom data types is possible, storing the state and the last operation. (Every change is decoded and transferred, so multiple
operations aren't needed). But at that point, the main benefits (simplicity, reuse of existing data types) are lost without gaining any advantage compared to state-based types (for example, still no capability to reset) except for the space requirements. (A per-node state isn't needed.)
operations aren't needed.) But at that point, the main benefits (simplicity, reuse of existing data types) are lost and no advantage gained compared to state-based types except for the space requirements. (A per-node state isn't needed.) For example, there's still no ability to reset.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check this edit.I was trying to break up a long sentence and too many parentheticals, and I wasn't 100% sure I got this right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it missing an "is"?

operations aren't needed.) But at that point, the main benefits (simplicity, reuse of existing data types) are lost and no advantage is gained compared to state-based types except for the space requirements. (A per-node state isn't needed.) For example, there's still no ability to reset.

Copy link
Contributor

Choose a reason for hiding this comment

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

The is is optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure? If you take out the phrase "the main benefits...are lost", the sentence becomes "But at that point no advantage gained compared to state-based types except for the space requirements..." I believe the way "and" is functioning there it requires the "is" to make the sentence valid. ChatGPT agrees and says it is the only way to ensure subject/verb agreement.

@ebgitelman
Copy link
Contributor Author

@jpe442 Overall this looked really great editorially. Most significant edits were for long sentences and overuse of parentheses. In some cases, commas would do just as well. In others, they made the sentence hard to follow. Generally, parentheticals reduce readability.

I did have a handful of queries, mostly to verify that my edit was correct.

@ebgitelman
Copy link
Contributor Author

@jpe442 Please review so I can get this merged. Thank you!

@ebgitelman
Copy link
Contributor Author

@djw-m @piano35-edb @jpe442 Thanks for the useful feedback. This should be ready to approve now. Thanks!

Copy link
Contributor

@djw-m djw-m left a comment

Choose a reason for hiding this comment

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

LGTM (but will it merge?)

@jpe442 jpe442 merged commit c945cee into develop Oct 22, 2024
2 checks passed
@jpe442 jpe442 deleted the docs/edits_to_pgd_pr5928 branch October 22, 2024 14:24
gvasquezvargas pushed a commit that referenced this pull request Oct 24, 2024
* Edits to Pgd/5/consistency/crdt/docs 287 PR5928

* Apply suggestions from code review

Co-authored-by: Dj Walker-Morgan <[email protected]>

---------

Co-authored-by: Dj Walker-Morgan <[email protected]>
gvasquezvargas pushed a commit that referenced this pull request Oct 24, 2024
* Edits to Pgd/5/consistency/crdt/docs 287 PR5928

* Apply suggestions from code review

Co-authored-by: Dj Walker-Morgan <[email protected]>

---------

Co-authored-by: Dj Walker-Morgan <[email protected]>
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.

3 participants