Skip to content

Fix idxInSync, WaitUntilIndexesAreSynced, and TestNames#301

Merged
decentral1se merged 2 commits intossbc:masterfrom
KyleMaas:fix-names-test
Jan 19, 2023
Merged

Fix idxInSync, WaitUntilIndexesAreSynced, and TestNames#301
decentral1se merged 2 commits intossbc:masterfrom
KyleMaas:fix-names-test

Conversation

@KyleMaas
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 added a commit that referenced this pull request Jan 19, 2023
@KyleMaas
Copy link
Copy Markdown
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