Skip to content

fix(Dgraph): Don't store start_ts in postings. #6127

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

Merged
merged 6 commits into from
Aug 4, 2020
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Aug 3, 2020

Clear out the start_ts field in the postings of deltas before they are
marshalled to avoid storing that field in disk. This field is only meant
to be used during in-memory processing.

Related to https://discuss.dgraph.io/t/start-ts-not-being-cleared-before-postings-are-written-to-disk/9146


This change is Reviewable

Clear out the start_ts and commit_ts fields in the postings of deltas
before they are marshalled to avoid storing those fields in disk. Those
fields are only meant to be used during in-memory processing.
@martinmr
Copy link
Contributor Author

martinmr commented Aug 3, 2020

This is what the debug tool is showing after and before the change.

After change.
➜  ~ dgraph debug -p=p -o=false -l 000006667269656e64000000000000000001 -y
[Decoder]: Using assembly version of decoder
I0803 15:15:18.477179 1280891 util_ee.go:126] KeyReader instantiated of type <nil>
Opening DB: p
badger 2020/08/03 15:15:18 INFO: All 1 tables opened in 0s
badger 2020/08/03 15:15:18 INFO: Replaying file id: 0 at offset: 593
badger 2020/08/03 15:15:18 INFO: Replay took: 1.72µs
==> key: 000006667269656e64000000000000000001. PK: {ByteType:0 Attr:friend Uid:1 HasStartUid:false StartUid:0 Term: Count:0 bytePrefix:0}
ts: 10 {item}{delta}
 postings: [uid:2 val_type:UID op:1  uid:3 val_type:UID op:1 ]  Uid: 2 Op: 1
 Uid: 3 Op: 1


badger 2020/08/03 15:15:18 INFO: Got compaction priority: {level:0 score:1.73 dropPrefixes:[]}

Before change
➜  ~ dgraph debug -p=p -o=false -l 000006667269656e64000000000000000001 -y
[Decoder]: Using assembly version of decoder
I0803 15:16:12.027965 1282216 util_ee.go:126] KeyReader instantiated of type <nil>
Opening DB: p
badger 2020/08/03 15:16:12 INFO: All 1 tables opened in 0s
badger 2020/08/03 15:16:12 INFO: Replaying file id: 0 at offset: 366
badger 2020/08/03 15:16:12 INFO: Replay took: 13.384µs
==> key: 000006667269656e64000000000000000001. PK: {ByteType:0 Attr:friend Uid:1 HasStartUid:false StartUid:0 Term: Count:0 bytePrefix:0}
ts: 4 {item}{delta}
 postings: [uid:2 val_type:UID op:1 start_ts:2  uid:3 val_type:UID op:1 start_ts:2 ]  Uid: 2 Op: 1
 Uid: 3 Op: 1


badger 2020/08/03 15:16:12 INFO: Got compaction priority: {level:0 score:1.73 dropPrefixes:[]}
badger 2020/08/03 15:16:12 INFO: Running for level: 0
badger 2020/08/03 15:16:12 INFO: LOG Compact 0->1, del 2 tables, add 1 tables, took 2.645601ms
badger 2020/08/03 15:16:12 INFO: Compaction for level: 0 DONE
badger 2020/08/03 15:16:12 INFO: Force compaction on level 0 done

@martinmr martinmr changed the title fix(Dgraph) Don't store start_ts or commit_ts in postings. fix(Dgraph): Don't store start_ts or commit_ts in postings. Aug 3, 2020
@martinmr martinmr changed the title fix(Dgraph): Don't store start_ts or commit_ts in postings. fix(Dgraph): Don't store start_ts in postings. Aug 4, 2020
@martinmr martinmr requested a review from a team August 4, 2020 19:30
Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Ok. But doesn't start_ts aid in trouble-shooting?

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Once it's on disk, the start ts is the same as the version of the key so it's just duplicate information. There are comments on the code saying start ts is just meant for in-memory processing but I guess at some point the code and the comments went out of sync.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)

@martinmr martinmr merged commit 9d7fc94 into master Aug 4, 2020
@martinmr martinmr deleted the martinmr/start-ts branch August 4, 2020 21:51
martinmr added a commit that referenced this pull request Aug 4, 2020
Clear out the start_ts field in the postings of deltas before they are
marshalled to avoid storing that field in disk. This field is only meant
to be used during in-memory processing.

Related to https://discuss.dgraph.io/t/start-ts-not-being-cleared-before-postings-are-written-to-disk/9146

(cherry picked from commit 9d7fc94)
martinmr added a commit that referenced this pull request Aug 4, 2020
Clear out the start_ts field in the postings of deltas before they are
marshalled to avoid storing that field in disk. This field is only meant
to be used during in-memory processing.

Related to https://discuss.dgraph.io/t/start-ts-not-being-cleared-before-postings-are-written-to-disk/9146

(cherry picked from commit 9d7fc94)
martinmr added a commit that referenced this pull request Aug 4, 2020
Clear out the start_ts field in the postings of deltas before they are
marshalled to avoid storing that field in disk. This field is only meant
to be used during in-memory processing.

Related to https://discuss.dgraph.io/t/start-ts-not-being-cleared-before-postings-are-written-to-disk/9146

(cherry picked from commit 9d7fc94)
jarifibrahim added a commit that referenced this pull request Aug 13, 2020
rahulgurnani pushed a commit that referenced this pull request Aug 13, 2020
parasssh pushed a commit that referenced this pull request Aug 13, 2020
martinmr added a commit that referenced this pull request Aug 17, 2020
Clear out the start_ts field in the postings of deltas before they are
marshalled to avoid storing that field in disk. This field is only meant
to be used during in-memory processing.

Related to https://discuss.dgraph.io/t/start-ts-not-being-cleared-before-postings-are-written-to-disk/9146
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants