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

Fix idxInSync, WaitUntilIndexesAreSynced, and TestNames #301

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

KyleMaas
Copy link
Contributor

@KyleMaas KyleMaas commented Jan 16, 2023

Requires ssbc/go-luigi#4, so be sure to add:

replace github.com/ssbc/go-luigi => ./../go-luigi

...or something like it to go.mod if that pull request hasn't yet been merged.

This fixes #251 and #250 by fixing idxInSync, WaitUntilIndexesAreSynced(), and TestNames(). I ran TestNames over 500 times with zero failures with this in place.

@KyleMaas
Copy link
Contributor Author

Oh, and the CI tests will fail with this, because they're not going to be using the updated version of Luigi.

@cryptix
Copy link
Member

cryptix commented Jan 16, 2023

Oh wow! I will try to have a look at this later this week.

re go.mod: once you pushed your working branch you should be able to go get $package@branch and not need a local replace anymore.

Copy link
Member

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

Incredible @KyleMaas! Yeh this matches up with all our discussion and thinking through this. Pretty damn smooth solution in the end 🤯 Will wait to hear from @cryptix for the final word but this LGTM 👏

@KyleMaas
Copy link
Contributor Author

Thanks! The first way I built this (which worked) was to pass in idxInSync as a WaitGroup pointer. But then I realized we can't reference count that to later query if indexes are caught up, which we need to gracefully fix this:

#293 (comment)

So I went with the callback method instead, which also opens up more options for other users of Luigi to implement status checking in other ways if they so choose. And now we can do "index busy" reference counting using atomic operations to give a status of how many indexes are out of sync. We can also use this to update the index status codes, so the callback method IMHO is far superior for flexibility.

Copy link
Member

@cryptix cryptix left a comment

Choose a reason for hiding this comment

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

Super down to trying this! Thanks for combing through this.

Left a comment on the luigi PR.

@decentral1se
Copy link
Member

decentral1se commented Jan 19, 2023

ssbc/go-luigi#4 has landed, let's rebase & merge here too 👏

I think go get github.com/ssbc/go-luigi@bd28e676fa9902104dac8620f332761346f968bd should summon the go.* file upgrades?

@decentral1se
Copy link
Member

Ah sure I can do the luigi work after the merge, thanks for all this work @KyleMaas!

@decentral1se decentral1se merged commit 0639c52 into ssbc:master Jan 19, 2023
decentral1se pushed a commit that referenced this pull request Jan 19, 2023
@KyleMaas
Copy link
Contributor Author

Excellent. Thanks!

@KyleMaas KyleMaas deleted the fix-names-test branch January 19, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

idxInSync is incorrect
3 participants