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

lib/model: Sort outgoing index updates by LocalVersion #3411

Closed
wants to merge 2 commits into from
Closed

lib/model: Sort outgoing index updates by LocalVersion #3411

wants to merge 2 commits into from

Conversation

calmh
Copy link
Member

@calmh calmh commented Jul 18, 2016

Purpose

Preparation for delta indexes. To review this part individually and not mix it up unnecessarily with the other changes. Adds some overhead to the initial index exchange, especially if the index is large, but not enough to affect the ManyFiles benchmark negatively in my testing.

Testing

The index sorter is fully covered by unit testing apart from the panic()ing lines. I've run the benchmarks and those work, so indexes are being sent.

Things to Review

I'm panicing in quite a few places in the sorter as I don't want to add error returns that would anyway be annoying to handle. I think the cases where there are panics are those where we would panic anyway - out of disk space, disk I/O failure, unable to marshal a struct, and so on. But I would appreciate an extra eye on it.

func (s *onDiskIndexSorter) Close() {
l.Debugf("onDiskIndexSorter %p closes", s)
s.db.Close()
os.RemoveAll(s.dir)
Copy link
Member

Choose a reason for hiding this comment

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

osutil?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 fixd

@AudriusButkevicius
Copy link
Member

The build failed?

@AudriusButkevicius
Copy link
Member

Build machine oom?

@calmh
Copy link
Member Author

calmh commented Jul 20, 2016

@st-jenkins retest this please

@calmh
Copy link
Member Author

calmh commented Jul 21, 2016

@AudriusButkevicius please take another look on this one :)

@AudriusButkevicius
Copy link
Member

@st-review merge

@st-review
Copy link

👌 Merged as 8ab6b60. Thanks, @calmh!

st-review pushed a commit that referenced this pull request Jul 21, 2016
@st-review st-review closed this Jul 21, 2016
@calmh calmh deleted the delta2 branch July 25, 2016 13:48
@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 22, 2017
@syncthing syncthing locked and limited conversation to collaborators Jul 22, 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

3 participants