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

lib/model: Properly schedule pull on reconnect (fixes #4504) #4508

Closed
wants to merge 2 commits into from
Closed

lib/model: Properly schedule pull on reconnect (fixes #4504) #4508

wants to merge 2 commits into from

Conversation

calmh
Copy link
Member

@calmh calmh commented Nov 13, 2017 •
edited

Purpose

We need to reset prevSeq so that we force a full check when someone reconnects - the sequence number may not have changed due to the reconnect. (This is a regression; we did this before f6ea2a7.)

Also add an optimization: we schedule a pull after scanning, but there is no need to do so if no changes were detected. This matters now because the scheduled pull actually traverses the database which is expensive.

This, however, makes the pull not happen on initial scan if there were no changes during the initial scan. Compensate by always scheduling a pull after initial scan in the rwfolder itself.

Testing

Previously failing integration test now passes

@calmh
lib/model: Properly schedule pull on reconnect (fixes syncthing#4504)
54ad710 
We need to reset prevSeq so that we force a full check when someone
reconnects - the sequence number may not have changed due to the
reconnect. (This is a regression; we did this before f6ea2a7.)

Also add an optimization: we schedule a pull after scanning, but there
is no need to do so if no changes were detected. This matters now
because the scheduled pull actually traverses the database which is
expensive.

This, however, makes the pull not happen on initial scan if there were
no changes during the initial scan. Compensate by always scheduling a
pull after initial scan in the rwfolder itself.
@imsodin
Copy link
Member

imsodin commented Nov 13, 2017

I think this makes prevSeq completely moot, as it will always be zero (except for failed pulls, but there it will always be different from the remote sequence, otherwise the pull wouldn't fail in the first place). This is actually just a consequence of f6ea2a7 that I missed together with the connection case that you fix here.

@imsodin
Copy link
Member

imsodin commented Nov 13, 2017 •
edited

Actually, the integration test in the issue works on my machine and I don't understand the issue conceptually: When pulling does not succeed, prevSeq is not updated, so the remote sequence should be higher and thus a pull should happen after reconnect. I missed that we consider a pull as successful, if needed files are not available when starting to pull - so the conceptual problem is understood. I am still puzzled why the integration test doesn't hang here.

@AudriusButkevicius
Copy link
Member

So, wazzup with zis?

@imsodin
Copy link
Member

imsodin commented Nov 14, 2017 •
edited

As a pull is now only scheduled when really necessary, the prevSeq is not needed any more and can be disposed of. It is set to 0 on every pull anyway. This is also nice because it reduces the complexity of the pull functions.

In code instead of words: imsodin@79693ae

@calmh
Copy link
Member Author

calmh commented Nov 17, 2017

I've incorporated that commit

@imsodin
Copy link
Member

imsodin commented Nov 17, 2017

I haven't really looked at the integration test changes - they run on my machine, that's good enough for me.

@imsodin
Copy link
Member

imsodin commented Nov 17, 2017

@st-review lgtm

@st-review
Copy link

@imsodin: Noted! Need another LGTM or explicit merge command.

@AudriusButkevicius
Copy link
Member

@st-review lgtm

@st-review
Copy link

👌 Merged as 5f4ed66. Thanks, @calmh!

@st-review st-review closed this Nov 17, 2017
st-review pushed a commit that referenced this pull request Nov 17, 2017
@calmh @AudriusButkevicius
lib/model: Properly schedule pull on reconnect (fixes #4504)
5f4ed66 
We need to reset prevSeq so that we force a full check when someone
reconnects - the sequence number may not have changed due to the
reconnect. (This is a regression; we did this before f6ea2a7.)

Also add an optimization: we schedule a pull after scanning, but there
is no need to do so if no changes were detected. This matters now
because the scheduled pull actually traverses the database which is
expensive.

This, however, makes the pull not happen on initial scan if there were
no changes during the initial scan. Compensate by always scheduling a
pull after initial scan in the rwfolder itself.

GitHub-Pull-Request: #4508
LGTM: imsodin, AudriusButkevicius
@calmh calmh deleted the pull branch November 17, 2017 12:37
@st-review st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018
@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 Nov 18, 2018
@syncthing syncthing locked and limited conversation to collaborators Nov 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion pr-merged Legacy label used in the past for pull requests merged to the main tree
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@calmh @imsodin @AudriusButkevicius @st-review