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.

Sign up for GitHub

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

Jump to bottom

kvdb/sqlbase: fix params used in randRetryDelay #7955

Merged
merged 4 commits into from
Sep 5, 2023
Merged

kvdb/sqlbase: fix params used in randRetryDelay #7955

merged 4 commits into from
Sep 5, 2023

Conversation

yyforyongyu
Copy link
Member

Realized the params used in randRetryDelay, causing us to always wait for 5s after the first attempt.

@yyforyongyu yyforyongyu self-assigned this Sep 4, 2023
guggero
guggero requested changes Sep 4, 2023
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Looks like the whole sqlbase package doesn't currently compile on master. The build isn't failing because we're still pinning a previous tag.

The main reason for this not being noticed is that we don't have any unit tests in that package. Otherwise we'd catch compilation errors of the current code in submodules (even if we don't reference it in the main go.mod)

@yyforyongyu
Copy link
Member Author

The main reason for this not being noticed is that we don't have any unit tests in that package. Otherwise we'd catch compilation errors of the current code in submodules (even if we don't reference it in the main go.mod)

I don't think it's the case as there is a TestInterface in kvdb/sqlite. Without updating go.mod I cannot get it run. How about we use replace github.com/lightningnetwork/lnd/kvdb => ./kvdb in our root go.mod?

@yyforyongyu yyforyongyu requested a review from guggero September 4, 2023 09:35
@guggero
Copy link
Collaborator

guggero commented Sep 4, 2023

The main reason for this not being noticed is that we don't have any unit tests in that package. Otherwise we'd catch compilation errors of the current code in submodules (even if we don't reference it in the main go.mod)

Then I think we need to change the way we run the unit tests. Or add the kvdb_sqlite build tag to the unit test tags?

I don't think it's the case as there is a TestInterface in kvdb/sqlite. Without updating go.mod I cannot get it run. How about we use replace github.com/lightningnetwork/lnd/kvdb => ./kvdb in our root go.mod?

We used to have local replaces. But the problem is that those don't propagate when lnd is used as a library. Then the version in the go.mod is used. So we often forgot to bump that version and then ended up needing to fix the go.mod after already having released a version. This forces us to update the go.mod to even get a new functionality into lnd in the first place.

@yyforyongyu yyforyongyu force-pushed the fix-rand-retry-delay branch 4 times, most recently from 7c877fb to 25462f5 Compare September 4, 2023 11:24
@yyforyongyu yyforyongyu added this to the v0.17.0 milestone Sep 4, 2023
guggero
guggero reviewed Sep 4, 2023
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Great fix, thanks a lot!

.github/workflows/main.yml Outdated Show resolved Hide resolved
scripts/unit_test_modules.sh Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@yyforyongyu yyforyongyu requested a review from guggero September 4, 2023 12:32
guggero
guggero approved these changes Sep 4, 2023
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice, LGTM 🎉

@yyforyongyu yyforyongyu mentioned this pull request Sep 4, 2023
Merged
@guggero
Copy link
Collaborator

guggero commented Sep 5, 2023 •
edited
Loading

Looks like there are more go module changes needed. We'll address them in #7944, maybe that should go in first.

@yyforyongyu yyforyongyu force-pushed the fix-rand-retry-delay branch 2 times, most recently from 822d668 to cc4afc3 Compare September 5, 2023 18:18
@yyforyongyu
Copy link
Member Author

Lasted push added a new command make tidy-module to run go mod tidy for all modules and a new action using make tidy-module-check to check modules are always updated.

@yyforyongyu
kvdb/sqlbase: fix params used in randRetryDelay
08f4940
@yyforyongyu
Makefile+action: add make unit-module to test submodules
5296509 
This commit adds a new command `make unit-module` to run unit tests for
submodules to avoid future build errors.
@yyforyongyu
kvdb/sqlbase: fix build errors
8813bc7 
We copy the `sqldb/sqlerrors.go` into `kvdb/sqlbase` to avoid import
cycles.
@yyforyongyu
github+Makefile: add new action make tidy-module-check
c568ba9 
This commit adds `tidy-module` and `tidy-module-check` to make sure the
modules are always tidy.
positiveblue
positiveblue approved these changes Sep 5, 2023
View reviewed changes
Copy link
Contributor

@positiveblue positiveblue left a comment

Choose a reason for hiding this comment

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

Thanks for adding the go mod tidy automation 💯

scripts/unit_test_modules.sh
#!/bin/bash

IGNORE="tools"
SUBMODULES=$(find . -mindepth 2 -name "go.mod" | cut -d'/' -f2 | grep -v "$IGNORE")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Roasbeef Roasbeef merged commit 84b2f2d into lightningnetwork:master Sep 5, 2023
@yyforyongyu yyforyongyu deleted the fix-rand-retry-delay branch September 6, 2023 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@positiveblue positiveblue positiveblue approved these changes

@guggero guggero guggero approved these changes

@Roasbeef Roasbeef Awaiting requested review from Roasbeef

Assignees

@yyforyongyu yyforyongyu

Labels
bug fix no-changelog
Projects
None yet
Milestone
v0.17.0
Development

Successfully merging this pull request may close these issues.
4 participants
@yyforyongyu @guggero @positiveblue @Roasbeef