Skip to content

update dgraph-io/ristretto to outcaste-io/ristretto#12559

Merged
gbbr merged 1 commit intoDataDog:mainfrom
mightyguava:main
Jul 15, 2022
Merged

update dgraph-io/ristretto to outcaste-io/ristretto#12559
gbbr merged 1 commit intoDataDog:mainfrom
mightyguava:main

Conversation

@mightyguava
Copy link
Contributor

@mightyguava mightyguava commented Jun 27, 2022

This change updates the dependency on dgraph-io/ristretto to outcaste-io/ristretto. The former is abandoned for the latter (see dgraph-io/ristretto#292 (comment)). This update also fixes an issue with an indirect dependency (glog) which pollutes the global space. See dgraph-io/ristretto#292 and #1310.

@mightyguava mightyguava requested a review from a team as a code owner June 27, 2022 23:28
@bits-bot
Copy link
Collaborator

bits-bot commented Jun 27, 2022

CLA assistant check
All committers have signed the CLA.

@gbbr
Copy link
Contributor

gbbr commented Jun 28, 2022

Hey there!

There is nothing in its README file suggesting that the library is abandoned. Do you have a link to an official announcement saying so?

I am not familiar with this library that you are proposing. I also don't see it announced anywhere as an official replacement. Neither am I aware of any issues with glog because of this library.

Can you please help clarify these points?

@mightyguava
Copy link
Contributor Author

mightyguava commented Jun 28, 2022

Hi @gbbr, I was going off the issue in https://github.com/dgraph-io/ristretto/pull/292, where the last comment says it is abandoned. It's not uncommon for open source projects to become unofficially abandoned, particularly when the authors left the company.

This is the first line in the README for the fork.

This is a fork of dgraph-io/ristretto, maintained by the original authors of the project and volunteers.

The issue with the glog library is that it pollutes the global flag namespace for Go. For us specifically, we are hitting a collision with the log_dir flag that glog declares. The linked issue above has details and more links.

@gbbr
Copy link
Contributor

gbbr commented Jun 29, 2022

It's not uncommon for open source projects to become unofficially abandoned, particularly when the authors left the company.
This is the first line in the README for the fork.

It's not, but in that case, I expect the source project to mention this in its README, and not a fork with 60 stars (compared to 4k)...

I was going off dgraph-io/ristretto#292 (comment)

Thanks! This helps!

The issue with the glog library is that it pollutes the global flag namespace for Go. For us specifically, we are hitting a collision with the log_dir flag that glog declares. The linked issue above has details and more links.

I see, this is also helpful context.

Happy to merge this. I will edit your PR's description and add a link to dgraph-io/ristretto#292 (comment). I hope that's ok with you.

@mightyguava
Copy link
Contributor Author

Happy to merge this. I will edit your PR's description and add a link to dgraph-io/ristretto#292 (comment). I hope that's ok with you.

Thanks! I probably should have included in the original PR description.

@mightyguava mightyguava reopened this Jun 29, 2022
@mightyguava
Copy link
Contributor Author

How do we get this merged? All checks seem to be passing except this mergefreeze?

@mx-psi
Copy link
Member

mx-psi commented Jun 30, 2022

How do we get this merged? All checks seem to be passing except this mergefreeze?

We are in the middle of releasing a new Agent version and for a few days only bugfixes can be merged to main (this is what the 'mergefreeze' check is about). We can merge your PR after this period is over.

@mightyguava
Copy link
Contributor Author

Hello again! Can this be merged now? Seems like the merge freeze is over

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

LGTM, will let @gbbr merge whenever ready. I triggered our Gitlab test suite (which has more tests than the public one)

@mightyguava
Copy link
Contributor Author

I can't seem to access these gitlab build results. But go mod tidy is clean for me for all 3 mod files on go version 1.18.3

@mx-psi
Copy link
Member

mx-psi commented Jul 13, 2022

I can't seem to access these gitlab build results. But go mod tidy is clean for me for all 3 mod files on go version 1.18.3

Can you run invoke -e tidy-all? That should tidy all modules. Note that we tidy with -compat=1.17, so that may explain any difference?

The dgraph version of the library is abandoned and the outcaste-io one
is now the official fork. The new version removes a dependency on the
glog library, that can cause conflicts due to global initialization, see outcaste-io/ristretto#3.

The same glog library has caused issues in this repo in the past as well DataDog#1310.
@mightyguava
Copy link
Contributor Author

mightyguava commented Jul 13, 2022

Ah, yeah, that made some changes to the go.sum, thanks

@mx-psi
Copy link
Member

mx-psi commented Jul 15, 2022

@gbbr Please merge whenever ready

@gbbr gbbr merged commit 08ca9ad into DataDog:main Jul 15, 2022
mx-psi added a commit that referenced this pull request Aug 3, 2022
@mx-psi
Copy link
Member

mx-psi commented Aug 3, 2022

Opened #12992 to revert this PR. The v0.2.0 tag is missing from the git repository, which breaks the build for people that can't use the Go proxy (e.g. due to requirements of their company or because they live in China). The PR can be accepted again if the tag is added back to the git repository.

gbbr pushed a commit that referenced this pull request Aug 4, 2022
@seanbachelder
Copy link

@mx-psi @mightyguava The tag has been added: outcaste-io/ristretto#4 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants