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

cmd/syncthing, lib/db, lib/model, lib/protocol: Implement delta indexes (fixes #438) #3427

Closed
wants to merge 3 commits into from

Conversation

calmh
Copy link
Member

@calmh calmh commented Jul 22, 2016

Purpose

This is the delta index implementation. There are a few main parts to it:

  • The "IndexID" stuff which is to ensure that we can detect when a given LocalVersion counter has reset or doesn't mean what we think it means. It also tells us whether the other side actually implements this part of the protocol or not (IndexID==0 meaning no support).
  • A tweak around how we handle incrementing LocalVersion in the database. I think this is simpler, and it's now just a regular incrementing counter per folder.
  • How we send indexes is changed, and it's now triggered from inside the code that takes care of receiving the ClusterConfig. There are a number of checks for the announced IndexID and MaxLocalVersion and if it checks out we start sending index updates only from that MaxLocalVersion and onwards.

Testing

I've ran the integration tests that are suitable, including the various "restart during transfer" tests and it checks out. I'm running it myself on my device since a week or so, and it works as far as I can tell.

Docs

Spec update: syncthing/docs#208. It's a backwards compatible change.

if deviceID == protocol.LocalDeviceID {
if f.LocalVersion > s.localVersion {
Copy link
Contributor

Choose a reason for hiding this comment

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

s.localVersion is never set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm? I set it on the line below your comment. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah you mean initialized perhaps? It's initialized to zero as we allocate the "s" struct above.

@AudriusButkevicius
Copy link
Member

@st-review merge

@AudriusButkevicius
Copy link
Member

AudriusButkevicius commented Jul 23, 2016

Nothing to pick on, clear and understandable.

🎉

@st-review
Copy link

👌 Merged as 47fa4b0. Thanks, @calmh!

@st-review st-review closed this Jul 23, 2016
st-review pushed a commit that referenced this pull request Jul 23, 2016
@calmh
Copy link
Member Author

calmh commented Jul 23, 2016

Lets hope it works

@calmh calmh deleted the delta3 branch July 23, 2016 13:47
@rumpelsepp
Copy link
Contributor

Wow! That's really exciting! Updating now...

@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jul 23, 2017
@syncthing syncthing locked and limited conversation to collaborators Jul 23, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants