Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

[new-backend] docs #4484

Merged
merged 7 commits into from
Oct 7, 2022
Merged

Conversation

kodebach
Copy link
Member

Basics

These points need to be fulfilled for every PR:

  • Short descriptions of your changes are in the release notes
    (added as entry in doc/news/_preparation_next_release.md which
    contains _(my name)_)
    Please always add something to the release notes.
  • Details of what you changed are in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildservers are happy. If not, fix in this order:
    • add a line in doc/news/_preparation_next_release.md
    • reformat the code with scripts/dev/reformat-all
    • make all unit tests pass
    • fix all memleaks
  • The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests for my code
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation
  • I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
  • I updated all meta data (e.g. README.md of plugins and METADATA.ini)
  • I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

Labels

If you are already Elektra developer:

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if the basics are fulfilled and you also
    say that everything is ready to be merged.

@kodebach kodebach changed the base branch from master to new-backend September 25, 2022 20:22
Copy link
Member Author

@kodebach kodebach left a comment

Choose a reason for hiding this comment

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

@markus2330 There are some open questions. Input is welcome.

Also @atmaxinger I made up some names for the hooks for notifications and spec. If you have better ideas go ahead and suggest them. Also, if you work on spec, you may be interesting in comment about metaspec://spec/remove.

doc/dev/backend-plugins.md Outdated Show resolved Hide resolved
doc/dev/backend-plugins.md Outdated Show resolved Hide resolved
doc/dev/backend-plugins.md Outdated Show resolved Hide resolved

Additionally, plugins may implement `elektra<Plugin>Commit` and `elektra<Plugin>Error`.
<!-- TODO: should commit and error really be separate? With the new elektraPluginGetPhase they could just be part of set... -->
Copy link
Member Author

Choose a reason for hiding this comment

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

@markus2330 What do you think? IMO commit/error should be merged back into set.

The current solution just creates unnecessary extra code in both libelektra-kdb

enum KdbSetFn
{
KDB_SET_FN_SET,
KDB_SET_FN_COMMIT,
KDB_SET_FN_ERROR,
};
static bool runSetPhase (KeySet * backends, Key * parentKey, const char * phase, bool blockErrors, enum KdbSetFn function)

and backend plugins

static int runPluginCommit (Plugin * plugin, KeySet * ks, Key * parentKey)
{
// TODO: provide way to access kdbCommit and name without kdbprivate.h
ksRewind (ks);
int ret = plugin->kdbCommit (plugin, ks, parentKey);
if (ret == ELEKTRA_PLUGIN_STATUS_ERROR)
{
if (keyGetMeta (parentKey, "error") == NULL)
{
addGenericError (parentKey, "kdbCommit", plugin->name);
}
}
return ret;
}
static int runPluginListCommit (PluginList * plugins, KeySet * ks, Key * parentKey)
{
for (PluginList * cur = plugins; cur != NULL; cur = cur->next)
{
if (runPluginCommit (cur->plugin, ks, parentKey) == ELEKTRA_PLUGIN_STATUS_ERROR)
{
return ELEKTRA_PLUGIN_STATUS_ERROR;
}
}
return ELEKTRA_PLUGIN_STATUS_SUCCESS;
}
int ELEKTRA_PLUGIN_FUNCTION (commit) (Plugin * plugin, KeySet * ks, Key * parentKey)

(there is more and it is and is all basically copy-paste from the set code)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is up to @flo91 to decide. I thought it is a good idea to have commit+error, in particular for the database backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it is a good idea to have commit+error, in particular for the database backend.

The phases still exist. The only question is whether we really need separate functions. Like I said it creates quite a bit of code duplication in many cases (because C doesn't have templates, generic, etc.).

If we take the backend plugin as the example, it basically a decision between what we have now and moving this

if (strcmp (phase, KDB_SET_PHASE_PRE_COMMIT) == 0)
{
return runPluginListCommit (handle->setPositions.precommit, ks, parentKey);
}
else if (strcmp (phase, KDB_SET_PHASE_COMMIT) == 0)
{
return runPluginCommit (handle->setPositions.commit, ks, parentKey);
}
else if (strcmp (phase, KDB_SET_PHASE_POST_COMMIT) == 0)
{
return runPluginListCommit (handle->setPositions.postcommit, ks, parentKey);
}

and this

if (strcmp (phase, KDB_SET_PHASE_PRE_ROLLBACK) == 0)
{
return runPluginListError (handle->setPositions.prerollback, ks, parentKey);
}
else if (strcmp (phase, KDB_SET_PHASE_ROLLBACK) == 0)
{
return runPluginError (handle->setPositions.rollback, ks, parentKey);
}
else if (strcmp (phase, KDB_SET_PHASE_POST_ROLLBACK) == 0)
{
return runPluginListError (handle->setPositions.postrollback, ks, parentKey);
}

into the if-else chain in the set function (and replacing run*Commit/run*Error with run*Set).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that code duplication in backend plugins should be the decisive factor here. IMHO more important is the clarity in resolvers. But I am open to both ways, having plugins with only open/get/set/close is more elegant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another (more dramatic) option would be to add the current phase as a 4th parameter to the get and set functions of plugins:

int elektraFooGet (Plugin * handle, ElektraGetPhase phase, KeySet * ks, Key * parentKey);
int elektraFooSet (Plugin * handle, ElektraSetPhase phase, KeySet * ks, Key * parentKey);

The it's also quite clear that phase should probably be handled in some way.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the phase can be communicated via the plugin handle better. Not every plugin is interested in the phase. In any case: there is doc/decisions/commit_function.md, which needs an update.

Copy link
Member Author

Choose a reason for hiding this comment

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

there is doc/decisions/commit_function.md, which needs an update.

Sure, but do we want a separate commit/error function or not. Because if not, I would just remove the file. I can create a new kdb_phases.md decisison, but I'm not sure we need it. It's already implemented and it is documented in doc/dev/backend-plugins.md

doc/dev/kdb-operations.md Outdated Show resolved Hide resolved
doc/dev/kdb-operations.md Outdated Show resolved Hide resolved
doc/dev/kdb-operations.md Outdated Show resolved Hide resolved
doc/dev/mountpoints.md Outdated Show resolved Hide resolved
doc/dev/mountpoints.md Outdated Show resolved Hide resolved
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Great work, hope to see this docu updates soon in new-backend!

doc/dev/backend-plugins.md Outdated Show resolved Hide resolved
doc/dev/backend-plugins.md Outdated Show resolved Hide resolved

Additionally, plugins may implement `elektra<Plugin>Commit` and `elektra<Plugin>Error`.
<!-- TODO: should commit and error really be separate? With the new elektraPluginGetPhase they could just be part of set... -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that code duplication in backend plugins should be the decisive factor here. IMHO more important is the clarity in resolvers. But I am open to both ways, having plugins with only open/get/set/close is more elegant.

doc/dev/kdb-operations.md Show resolved Hide resolved
doc/dev/kdb-operations.md Show resolved Hide resolved
doc/dev/kdb-operations.md Outdated Show resolved Hide resolved
doc/dev/mountpoints.md Outdated Show resolved Hide resolved
doc/dev/mountpoints.md Outdated Show resolved Hide resolved
doc/dev/mountpoints.md Outdated Show resolved Hide resolved
src/libs/elektra/kdb.c Outdated Show resolved Hide resolved
@kodebach
Copy link
Member Author

@markus2330 I didn't have time to incorporate your review yet. But I did now look through most of the docs and removed some more things.

I didn't remove much and instead added clear warnings ("Outdated"). All these files should be updated before the release. I didn't remove them because they are all still mostly correct and I didn't want to rewrite them all (because just removing something the middle probably creates more confusion).

I didn't really look closely at the ALLCAPS.md files, since they all seems mostly unrelated to the new-backend changes.

For the manpages (especially elektra-*) I didn't do anything. I didn't just want to remove them because the might still contain some useful stuff. But anything related to mountpoints/backends probably needs to rewritten.

Another big reason why I removed so little is: I think the old stuff can be used as reference for the person writing the updated docs. Only the outdated parts really need to be replaced.

@markus2330
Copy link
Contributor

@markus2330 I didn't have time to incorporate your review yet.

I think it is urgent as this PR we definitely need before merging new-backend. And FLOSS PRs are coming soon.

But I did now look through most of the docs and removed some more things.

Thank you very much 🥇 🎆

Will have a look soon. Or is it better we simply merge it and I'll directly review the new-backend PR?

I didn't remove much and instead added clear warnings ("Outdated"). All these files should be updated before the release.

Please also add TODO NEW-BACKEND to make it easier to grep for it. (Warning could be ok to release.)

I didn't remove them because they are all still mostly correct and I didn't want to rewrite them all (because just removing something the middle probably creates more confusion).

Yes, needs to be decided case-per-case.

I didn't really look closely at the ALLCAPS.md files, since they all seems mostly unrelated to the new-backend changes.

Probably fine.

For the manpages (especially elektra-*) I didn't do anything. I didn't just want to remove them because the might still contain some useful stuff. But anything related to mountpoints/backends probably needs to rewritten.

It is also enough if you mark the whole file for now. But all outdated content should be marked.

Another big reason why I removed so little is: I think the old stuff can be used as reference for the person writing the updated docs.

As already said, you need to write most of the new-backend-specific docu as nobody else knows the details.

@kodebach
Copy link
Member Author

kodebach commented Oct 6, 2022

I think it is urgent as this PR we definitely need before merging new-backend. And FLOSS PRs are coming soon

Do you have a specific date? I'll try to incorporate the review today/tomorrow. And I'll merge master into new-backend as well. To make it (technically) possible to merge #4187.

IMO we should just merge what is there in new-backend ASAP, even if you are not fully satisfied with the docs. I'll do my best to make more updates in the next weeks.

Or is it better we simply merge it and I'll directly review the new-backend PR?

Again I don't know when the deadline is (i.e. when FLOSS needs things merged). But if you really want to review the entire #4187, better start right now. It is big and if the review goes like most of our reviews, there will be extra discussions...

Please also add TODO NEW-BACKEND to make it easier to grep for it.

Will do

As already said, you need to write most of the new-backend-specific docu as nobody else knows the details.

NO. There is already lots of docs for the new-backend changes: doc/dev/kdb-operations.md, doc/dev/mountpoints.md, doc/dev/backend-plugins.md. In fact some of those were there before I implemented the changes. I honestly have no idea why you either purposefully ignore those files, or seem to forget them all the time.

Based on those docs, many of the other less low-level docs can easily be updated by other people. For example, AFAICT doc/dev/algorithm.md is supposed be similar to what doc/dev/kdb-operations.md describes, but less like a specification.

There are also multiple other people implementing things for new-backend. People who do that was part of their thesis, while I spend time on it, completely unrelated to my thesis topic...

@kodebach
Copy link
Member Author

kodebach commented Oct 6, 2022

@markus2330 I have now updated and rebased this PR. Please to a quick, cursory check and merge the PR, unless there is some really major issue. I'll do more minor update in another PR

@kodebach kodebach marked this pull request as ready for review October 7, 2022 00:54
@kodebach kodebach merged commit 624d4ed into ElektraInitiative:new-backend Oct 7, 2022
@kodebach
Copy link
Member Author

kodebach commented Oct 7, 2022

I merged this PR since there is still #4187 which can still be reviewed.

@markus2330
Copy link
Contributor

Do you have a specific date?

No, it is asap. People may fork their repo any time.

IMO we should just merge what is there in new-backend ASAP

I fully agree!

even if you are not fully satisfied with the docs. I'll do my best to make more updates in the next weeks.

Thank you! ❤️

NO. There is already lots of docs for the new-backend changes: doc/dev/kdb-operations.md, doc/dev/mountpoints.md, doc/dev/backend-plugins.md. In fact some of those were there before I implemented the changes. I honestly have no idea why you either purposefully ignore those files, or seem to forget them all the time.

Based on those docs, many of the other less low-level docs can easily be updated by other people. For example, AFAICT doc/dev/algorithm.md is supposed be similar to what doc/dev/kdb-operations.md describes, but less like a specification.

E.g. doc/dev/kdb-operations.md is a useful description how to implement a backend. But there are many other viewpoints, e.g. the algorithms etc. which are not yet documented. We need easier understandable docu for new contributors, at least at the level as we had before.

There are also multiple other people implementing things for new-backend. People who do that was part of their thesis, while I spend time on it, completely unrelated to my thesis topic...

It isn't a perfect match for anyone, as it is a huge left-over and there cannot be a thesis about "finishing others peoples job".

Anyway, I am very grateful for your amazing&huge contributions on the most important topics.

@kodebach
Copy link
Member Author

IMO we should just merge what is there in new-backend ASAP

I fully agree!

The problem is it seems we disagree about the definition of "ASAP". IMO ASAP is basically now (maybe when I have disabled the failing tests). I think you want a bit more work done, or at least complete your review. I understand that, but any delay will lead to people starting to work on master and then the merge could be quite annoying for them.

We need easier understandable docu for new contributors, at least at the level as we had before.

doc/dev/algorithm.md will still exist and that's why I didn't delete it. It just needs to be updated. I didn't have time for that, and I believe that updating it could also be done by someone else who has read the newly added docs. The general idea of the algorithm is still the same, only some details changed, but those changes should be obvious from conflicts between new and old docs.

I have now also updated the doxygen docs for kdbGet and kdbSet so there is even more information to find what has changed.

It isn't a perfect match for anyone, as it is a huge left-over and there cannot be a thesis about "finishing others peoples job".

Of course, but if everyone does a bit, it's much easier for everyone.

@kodebach kodebach mentioned this pull request Oct 24, 2022
22 tasks
@mpranj mpranj added this to the 0.9.12 milestone Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants