kdb check: should check for commit symbol

[markus2330]

libs/tools/src/plugin.cpp should also check for the presence and consistency of the "commit" function.

[stale[bot]]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

[markus2330]

@vLesk is this already in your PR?

[stale[bot]]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

[markus2330]

@kodebach is this already done in the new-backend branch?

[kodebach]

It's somewhat done yes. But the setup in new-backend would actually fit better, if kdbCommit and kdbError are merged with kdbSet, since they are just phases of the set operation and cannot be called separately. Because we have a separate way to communicate the phase to the plugin in new-backend, we don't need the the separate kdbCommit/kdbError functions.

[markus2330]

Yes, we need to decide: either phases or separate functions.

@atmaxinger @flo91 did you already look at the phases document?

@flo91 Does kdbCommit/kdbError sound convincing for your use-case (it is actually modeled after databases) or do you think you can work with phases as well?

[kodebach]

Yes, we need to decide: either phases or separate functions.

"separate functions" would also mean separate exports for get-prestorage, get-storage, get-poststorage etc. instead of a single get.

AFAIK you (@markus2330) are always in favour of fewer exports, so this doesn't look like a solution to me.

Does kdbCommit/kdbError sound convincing for your use-case (it is actually modeled after databases) or do you think you can work with phases as well?

commit/error still exist when we go with phases. The change would just be that plugins only have open/get/set/close functions (matching kdbOpen/kdbGet/kdbSet/kdbClose). Since plugins could be called multiple times (especially backends) during a single kdbGet/kdbSet, we added a elektraPluginGetPhase that let's plugins know what kind of call (="phase") this is.

did you already look at the phases document?

@markus2330 Did you? Not to be rude, but your comments seem like "phases or separate functions" would be a fundamental change, when in fact "phases" is just a term I made up to describe what is already happening inside kdbGet/kdbSet on master. The only difference is how this information is conveyed to the plugins.

To briefly summarize: There are 4 "operations" open/get/set/close (matching kdbOpen/kdbGet/kdbSet/kdbClose) and some of them (get/set) have multiple "phases" in which we delegate to plugins an expect certain things to happen.

---

Essentially what "phases or separate functions" boils down to is, where the dispatch happens:

// each exported and called by `libelektra-kdb`
// only what the plugin supports exists
int elektraPluginSetResolver();
int elektraPluginSetPreStorage();
int elektraPluginSetStorage();
int elektraPluginSetPostStorage();
int elektraPluginSetPreCommit();
int elektraPluginSetCommit();
int elektraPluginSetPostCommit();
int elektraPluginSetPreRollback();
int elektraPluginSetRollback();
int elektraPluginSetPostRollback();

VS

// only this is called by `libelektra-kdb`
// exists as soon as the plugin supports at least one set phase
int elektraPluginSet() {
   phase = elektraPluginGetPhase();
   switch (phase) {
      // Note: not all of these functions need to exist, sometimes the plugin may return directly or the code could be inline
      case RESOLVER: return elektraPluginSetResolver();
      case PRE_STORAGE: return elektraPluginSetPreStorage();
      case STORAGE: return elektraPluginSetStorage();
      case POST_STORAGE: return elektraPluginSetPostStorage();
      case PRE_COMMIT: return elektraPluginSetPreCommit();
      case COMMIT: return elektraPluginSetCommit();
      case POST_COMMIT: return elektraPluginSetPostCommit();
      case PRE_ROLLBACK: return elektraPluginSetPreRollback();
      case ROLLBACK: return elektraPluginSetRollback();
      case POST_ROLLBACK: return elektraPluginSetPostRollback();
   }
}

Note: "exported" in this context doesn't necessarily mean exported symbol in the SO, it could (and probably will) be just an entry in the contract (in which case KeySet * elektraPluginContract() should be the only actual exported symbol)

[markus2330]

"separate functions"

Yes, I mean error and commit functions, so yes the two possibilities are what you later describe with "VS".

What purpose do error and commit currently fulfill in the new-backend branch?

[kodebach]

the two possibilities are what you later describe with "VS".

To be clear, if we go with separate functions that would apply to e.g. poststorage too. I extended the example above (and replaced "error" with the actually used term "rollback").

What purpose do error and commit currently fulfill in the new-backend branch?

Exactly the same as in the old one. To repeat exactly what I wrote in the previous comment:

in fact "phases" is just a term I made up to describe what is already happening inside kdbGet/kdbSet on master. The only difference is how this information is conveyed to the plugins.

To briefly summarize: There are 4 "operations" open/get/set/close (matching kdbOpen/kdbGet/kdbSet/kdbClose) and some of them (get/set) have multiple "phases" in which we delegate to plugins an expect certain things to happen.

[kodebach]

Also discussed in this comment chain: https://github.com/ElektraInitiative/libelektra/pull/4484#discussion_r979453510

IMO we should undo the separately exported commit function and also remove the separately exported error function and merge both of them back into the set function. At the end both "commit" and "error" are part of "set" and not something separate that can be used standalone. As discussed above this would also mean the open/get/set/close API of plugins would match the kdbOpen/kdbGet/kdbSet/kdbClose API.

If we do this, reintegrating commit and error into set could be a FLOSS task. It should just be copying existing code around and changing some names.

[markus2330]

I am still waiting for a comment of @flo91 on this one, as commit/error was designed for the database plugin. It is also very helpful for resolver plugins. Anyway, let us give @flo91 the next word, nothing is urgent here.

[kodebach]

Actually it is kind of urgent, so I know what to document in #4484, see also https://github.com/ElektraInitiative/libelektra/pull/4484#discussion_r984908094 (whole chain). I will of course document the current implementation, but I'd like to also document, if we plan to change this.

[markus2330]

@kodebach Sorry, missed this comment. Other than in doc/decisions we don't write in the repo anything what we would like to do/change as you never know if and when changes will happen. It is only confusing if parts of decisions are scattered around everywhere. Did you finally write something about commit somewhere? If yes, where?

@flo91 please create a decision about this Commit/Error functions.

[kodebach]

@markus2330 Can you please at least try to check what is already there, before delegating work to other people.

doc/decisions/commit_function.md was already part of #4187. You even mentioned it in https://github.com/ElektraInitiative/libelektra/pull/4484#discussion_r990662169 so no idea what you want from flo91 here.

Did you finally write something about commit somewhere? If yes, where?

No I did not write anything new beyond the conversations we had in comments here and in https://github.com/ElektraInitiative/libelektra/pull/4484#discussion_r979453510, because I don't know what we want to do. In https://github.com/ElektraInitiative/libelektra/pull/4187#discussion_r991107372 you again preferred just having phases over separate functions.

It is only confusing if parts of decisions are scattered around everywhere.

In general I agree. In this case I disagree. We both know that whatever we decide to do here, it has to be implemented and properly documented before the next release. So implementing and documenting it one way now only to change it in a few weeks is a pointless waste of time. That's why I wanted to document it the way it will be in the end and add a note that this still has to be implemented.

[markus2330]

I agree that we need to have a decision and not doing something now and revert it later (unless assumptions change like happened in doc/decisions/commit_function.md where nothing was known of phases). I disagree that I need to comment about anything, I don't want to create even more prejudice here. I'll continue the discussion when @flo91 starts the decision process.

[github-actions[bot]]

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

[github-actions[bot]]

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart: