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.

Sign up for GitHub

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

Jump to bottom

refactor: Remove first CRDT byte from field encoded values #1622

Merged
merged 4 commits into from
Jul 12, 2023
Merged

refactor: Remove first CRDT byte from field encoded values #1622

merged 4 commits into from
Jul 12, 2023

Conversation

islamaliev
Copy link
Contributor

Relevant issue(s)

Resolves #1620

Description

Remove 0th-byte CRDT type from field encoded values

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Unit and integration tests

Specify the platform(s) on which this was tested:

  • MacOS

@islamaliev islamaliev added area/datastore Related to the datastore / storage engine system area/crdt Related to the (Merkle) CRDT system refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels Jul 11, 2023
@islamaliev islamaliev self-assigned this Jul 11, 2023
@codecov
Copy link

codecov bot commented Jul 11, 2023 •
edited

Codecov Report

Patch coverage: 40.00% and project coverage change: -0.11 ⚠️

Comparison is base (ae16a95) 76.09% compared to head (7c3f646) 75.98%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1622      +/-   ##
===========================================
- Coverage    76.09%   75.98%   -0.11%     
===========================================
  Files          192      192              
  Lines        19856    19852       -4     
===========================================
- Hits         15108    15084      -24     
- Misses        3717     3731      +14     
- Partials      1031     1037       +6
Flag Coverage Δ
all-tests 75.98% <40.00%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
db/fetcher/encoded_doc.go 62.26% <36.84%> (-0.47%) ⬇️
core/crdt/lwwreg.go 75.51% <100.00%> (-0.49%) ⬇️

... and 5 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae16a95...7c3f646. Read the comment docs.

AndrewSisley
AndrewSisley approved these changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

LGTM - cheers for getting rid of this Islam :)

docs/data_format_changes/i1620-remove-first-crdt-byte.md
@@ -0,0 +1,3 @@
# Remove the first CRDT byte from field encoded values

The first CRDT byte was legacy code and no longer necessary as we have this information independently available via the client.FieldDescription, since the FieldDescription.Typ is the exact same value.
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: For the future (I wouldn't bother changing this now as atm it doesnt really matter), but these docs are really for our users, and I think this is worded a little too internally and might not be very helpful for external persons.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I always saw this as internal dev information. It's an interesting point of view :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is kind of like a change-log, of really really important changes. Users will likely want to know when, how and maybe why their persisted data has been broken between versions, regardless as to whether or not we have any migrations to help them migrate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll have to surface these elsewhere because I'm certain that users won't come looking for those in a repo sub folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, the current location just allows us to build up the history and detect and document when we do actually make breaking changes. These docs will later need to be properly incorporated into externally friendly release documentation.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's should be healthy balance of both user and dev. User so they know why it changed, and dev so they know how it changed

fredcarle
fredcarle approved these changes Jul 11, 2023
View reviewed changes
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM!

@jsimnz
Copy link
Member

jsimnz commented Jul 11, 2023

Haven't reviewed but considering the thumbs up from Andy and Fred it's likely good to go.

Just wait till @AndrewSisley has merged the lens PR before merging this, so he doesn't have the conflicts (you've pulled the short straw on this one) 👍

@AndrewSisley
Copy link
Contributor

AndrewSisley commented Jul 12, 2023 •
edited

Haven't reviewed but considering the thumbs up from Andy and Fred it's likely good to go.

Just wait till @AndrewSisley has merged the lens PR before merging this, so he doesn't have the conflicts (you've pulled the short straw on this one) +1

@islamaliev Don't wait for me to merge, the conflicts will be small and the progression of the Lens PR is taking its time. No need to block this.

@islamaliev islamaliev force-pushed the feat/islam/remove-zero-crdt-byte branch from 1b929dc to 7c3f646 Compare July 12, 2023 13:57
@islamaliev islamaliev merged commit 6978664 into develop Jul 12, 2023
12 checks passed
@islamaliev islamaliev deleted the feat/islam/remove-zero-crdt-byte branch July 12, 2023 14:30
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@islamaliev
refactor: Remove first CRDT byte from field encoded values (sourcenet…
d225805 
…work#1622)

## Relevant issue(s)

Resolves sourcenetwork#1620

## Description

Remove 0th-byte CRDT type from field encoded values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredcarle fredcarle fredcarle approved these changes

@AndrewSisley AndrewSisley AndrewSisley approved these changes

@jsimnz jsimnz Awaiting requested review from jsimnz

Assignees

@islamaliev islamaliev

Labels
area/crdt Related to the (Merkle) CRDT system area/datastore Related to the datastore / storage engine system refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove 0th-byte CRDT type from field encoded values
4 participants
@islamaliev @jsimnz @AndrewSisley @fredcarle