search

canonical / dqlite

Embeddable, replicated and fault-tolerant SQL engine.
https://dqlite.io
Other
3.8k stars 214 forks source link

Git layout for v1.15.0 #521

Closed freeekanayaka closed 1 year ago

freeekanayaka commented 1 year ago

It seems that the v1.15.0 tag has been put on a separate release-v1.15.0 branch, instead of putting it on master like we usually do.

Since afaics the release-v1.15.0 branch has the same content of master, minus the new dqlite_server_xxx APIs, I presume the intention was to leave out the changes from #514 onwards.

However this effectively is a fork of master, with a few possibly unfortunate consequences:

I think it'd be fine to simply include the recent dqlite_server_xxx code in the v1.15.0 release, cutting v1.15.0 from master as usual.

If desired, we could introduce a DQLITE_EXPERIMENTAL, similar to SQLITE_EXPERIMENTAL, see the docs and the code.

If there are changes that we don't want to include in the next release,y the probably shouldn't be committed to master in the first place (but I don't think we need to do that, since we merge only working changes that don't make master "unreleasable").

cole-miller commented 1 year ago

I agree that the side effects you list are unfortunate. In the future I'll probably approach this problem by putting the incomplete API surface behind an undocumented #ifdef DQLITE_INCOMPLETE.

freeekanayaka commented 1 year ago

I agree that the side effects you list are unfortunate. In the future I'll probably approach this problem by putting the incomplete API surface behind an undocumented #ifdef DQLITE_INCOMPLETE.

Is there a specific reason you don't want the new APIs to be part of this v1.15.0 release?

It seems to me that they are complete and could already be used, and actually they are what new consumers should use, since they offer better functionality/signatures than the dqlite_node_xxx, which we should already deprecate . These new APIs don't require a client or anything like that.

cole-miller commented 1 year ago

I don't have a specific reason other than giving the new interfaces the maximum time to "cook" before making them accessible in an official release. My upcoming implementation of the remaining parts of the C client API will involve some changes to dqlite_server, and there are some kinds of testing that I would like to do using the full C client API that I suspect will shake out more bugs in the already-merged code.

freeekanayaka commented 1 year ago

Possible bugs are fine, they are possible as in any release, so they shouldn't be a blocker.

Are you envisioning changes to the dqlite_server_xxx APIs or changes to the implementation? The latter case is also fine and normal, not blocking in any way.

I would expect no change to the existing dqlite_server_xxx APIs (adding new APIs in the future is always ok).

I'd propose to just add a no-op DQLITE_EXPERIMENTAL "tag" (or simply prefix the docstring with the word "Experimental"), in the worst case that small tweaks to the signatures are needed that we're not seeing now.

That would leave us more time before we declare those interfaces as final and stable, possibly taking in user input after the release of the experimental version. Having a flag-day switch like #ifdef DQLITE_INCOMPLETE would mean that we'd commit to get them perfect with no chance of small tweaks (we'd also have to multiply the test matrix to run with and without DQLITE_INCOMPLETE).

I'm mostly concerned by having a release-v1.15.0 branch that is an oddball which we have to keep around indefinitely, and by not having the v1.15.0 not be part of the master branch like all other release tags.

Would it be ok for you to mark these APIs as experimental and cut a regular v1.15.0 release from master?

I think it'd be fine to delete the current v1.15.0 tag since it's so recent, and there won't be actual changes for users.

cole-miller commented 1 year ago

I'm not comfortable moving a published tag, but I wouldn't mind introducing DQLITE_EXPERIMENTAL for functions that are implemented but not yet ready to be stabilized. I don't think that approach is right for functions that haven't even been implemented yet, as much of the client API (the non-dqlite_server parts) hasn't. I guess it may have been a mistake to merge the changes to dqlite.h without corresponding sane implementations, although it made sense to me at the time.

freeekanayaka commented 1 year ago

I'm not comfortable moving a published tag

I understand, but what are we going to do with the next release though (say v1.16.0)? It will not have v1.15.0 as git parent? Will we need to keep the release-v1.15.0 branch around indefinitely? It seems odd to me.

Retagging seems still fine for now, so we keep commit/release history in order long-term. We're not going breaking code or anything like that. Just my 2c.

but I wouldn't mind introducing DQLITE_EXPERIMENTAL for functions that are implemented but not yet ready to be stabilized. I don't think that approach is right for functions that haven't even been implemented yet, as much of the client API (the non-dqlite_server parts) hasn't. I guess it may have been a mistake to merge the changes to dqlite.h without corresponding sane implementations, although it made sense to me at the time.

Ok, we could use DQLITE_INCOMPLETE for those, or something equivalent. In practice, I doubt we will have users complaining about that either way, so although we might not do it again in the future, most probably nobody is going to be mad at that in the present, whether we use DQLITE_INCOMPLETE or not.

freeekanayaka commented 1 year ago

We can also remove the client functions temporarily from master, and re-add them little by little as they get implemented.

cole-miller commented 1 year ago

How about this for a compromise: