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

Prepare to merge dqlite-next into master #642

Merged
merged 10 commits into from
Apr 25, 2024

Conversation

cole-miller
Copy link
Contributor

Let's do all our work on one branch, with experimental stuff (like thread pool and vfs2 integration) guarded behind #ifdef DQLITE_NEXT. This will avoid a big merge headache down the line.

Signed-off-by: Cole Miller [email protected]

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.56%. Comparing base (4d74efa) to head (ec33e4f).
Report is 3 commits behind head on dqlite-next.

Additional details and impacted files
@@             Coverage Diff              @@
##           dqlite-next     #642   +/-   ##
============================================
  Coverage        80.56%   80.56%           
============================================
  Files              196      196           
  Lines            28300    28304    +4     
  Branches          5297     5300    +3     
============================================
+ Hits             22799    22804    +5     
+ Misses            3808     3767   -41     
- Partials          1693     1733   +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cole-miller
Copy link
Contributor Author

@just-now I think this is good to go, would appreciate you confirming that I didn't miss anything important when adding ifdefs.

@cole-miller cole-miller force-pushed the integrate branch 2 times, most recently from 176b4fb to dbdf8fc Compare April 24, 2024 14:37
Copy link
Contributor

@letFunny letFunny left a comment

Choose a reason for hiding this comment

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

It is a little bit tricky to review but I tried to have master and this PR side by side to see if they had the same behavior when DQLITE_NEXT was not defined. Looks good to me apart from one comment I included, thanks!

Makefile.am Show resolved Hide resolved
@@ -186,9 +188,11 @@ int dqlite__init(struct dqlite_node *d,
err_after_raft_transport_init:
raftProxyClose(&d->raft_transport);
err_after_pool_init:
#ifdef DQLITE_NEXT
pool_close(&d->pool);
pool_fini(&d->pool);
err_after_loop_init:
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be wrong here but I think the label that exists in master at the moment is err_after_loop_init so the if def should be:

....
err_after_raft_transport_init:
	raftProxyClose(&d->raft_transport);
#ifdef DQLITE_NEXT
err_after_pool_init:
	pool_close(&d->pool);
	pool_fini(&d->pool);
#endif
err_after_loop_init:
	uv_loop_close(&d->loop);
....

Copy link
Contributor Author

@cole-miller cole-miller Apr 25, 2024

Choose a reason for hiding this comment

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

That way of doing it doesn't quite work as-is because on dqlite-next there is a goto err_after_pool_init in the error branch after raftProxyInit above. We would have to change that to

        rv = raftProxyInit(&d->raft_transport, &d->loop);
        if (rv != 0) {
#ifdef DQLITE_NEXT
                goto err_after_pool_init;
#else
                goto err_after_loop_init;
#endif
        }

The way I've written it looks kind of weird but gets away with only one ifdef, which I think is nice. Thoughts?

@cole-miller
Copy link
Contributor Author

Pushed 24a4879 to codify that dqlite-next relies (will rely) on using the bundled raft sources and headers, since we will be making incompatible changes like #639.

Copy link
Contributor

@just-now just-now left a comment

Choose a reason for hiding this comment

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

Looks good to me.

src/leader.c Outdated Show resolved Hide resolved
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
I don't see the point of testing without LIBDQLITE_TRACE in the
environment.

Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Let's not make users of stable dqlite pay this cost in startup time and
thread count.

Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
@cole-miller cole-miller merged commit a544e69 into canonical:dqlite-next Apr 25, 2024
13 checks passed
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.

3 participants