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

vendor: Update vendored goleveldb, and check if other dependencies should be updated, too #1768

Closed
beorn7 opened this Issue Jun 28, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@beorn7
Copy link
Member

beorn7 commented Jun 28, 2016

Before we release 1.0.0, we should get our vendored dependencies up-to-date and give them a test run.
In particular, goleveldb has some fixes and performance improvements upstream.

@beorn7 beorn7 added this to the v1.0.0 milestone Jun 28, 2016

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jul 3, 2016

Any particular reason why?
I don't see too much urgency here. We should check whether there are security or bug fixes affecting our code path but in general this is always has potential for regressions.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Jul 3, 2016

I did check. That made me file this issue. And that's what I tried to express by "goleveldb has some fixes and performance improvements upstream".
Obviously, we should canary the changes, but given that we have performance problems with the indexes, I think it's worth it. I cannot really judge the actual impact of the bug fixes, as we had very few incidents reported that might have been triggered by goleveldb bugs. But the commit descriptions sounded pretty relevant to me, so I wouldn't like to release a 1.0.0 software with know bugs that are even known to be fixed upstream.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jul 3, 2016

Yes, I took it as updating everything rather than the selected ones. So the question should've rather been whether you want to update these specific ones or just everything. I wasn't so clear on that.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Jul 3, 2016

I see. Then I have to apologize for my lack of clarity. It really sounds in the OP as if I want to do a blanket update of everything. What I should have said: Let's do goleveldb in any case, and while we are on it, let's check the other dependencies if there is a reason to update. I'll change the title accordingly.

@beorn7 beorn7 changed the title vendor: Update vendored dependencies vendor: Update vendored goleveldb, and check if other dependencies should be updated, too Jul 3, 2016

@beorn7 beorn7 referenced this issue Jul 4, 2016

Merged

Update vendoring #1785

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jul 4, 2016

Closed via #1785

@fabxc fabxc closed this Jul 4, 2016

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.