doc: add decision docs for decision meeting 15.07.2021 #3919

lawli3t · 2021-07-04T21:56:17Z

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:

Documentation is introductory, concise, good to read and describes everything what the PR does
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

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.

Closes #3839

lawli3t · 2021-07-04T21:59:28Z

@kodebach in this comment #3750 (comment) you mentioned something about

changes to the backend and global plugin stuff

Would you like to add this to the agenda of this meeting?

kodebach · 2021-07-04T22:54:09Z

changes to the backend and global plugin stuff

Would you like to add this to the agenda of this meeting?

I "discussed" this with @markus2330. We both think that this would probably be too much and is somewhat unrelated to the rest of the agenda (which is more concerned with the C API). Despite that I would still like to briefly discuss the changes of #3693 at the end of the meeting. I would just give a brief overview of what the idea is to get some quick feedback on the general concepts. For a more in-depth discussion, we will definitely need a separate meeting and people will probably have to read the new documentation first.

markus2330

Thank you so much for the preparation of the meeting!

markus2330 · 2021-07-14T18:57:48Z

doc/decisions/error_handling.md

+
+## Problem
+
+Currently, a lot of functions return the error value, regardless of error. These poses some problems, for instance authors of bindings, where it is hard to tell which error exactly occured. A big improvement to this would be returning different error codes for different types of errors, in order to be able to provide better information in cases of errors. Some functions also make use of an `errorKey`. While this seems like a decent approach, to provide additional informations in cases of error, it is currently very badly documented. It is not really transparent for the users of the function, what kind of information is included in the `errorKey` and even which messages could possibly be returned.


Imho here are several issues described (errors of Key/KeySet, errors of KDB). Please also have a look at earlier attempts to errors we already had: using errno (mapping of errors was troublesome).

doc/decisions/iterators.md

doc/decisions/key_string_return_value.md

doc/decisions/reference_counting.md

markus2330 · 2021-07-14T19:04:40Z

doc/decisions/reference_counting.md

+- use `keyDel` to dispose of a `Key *` (whether the `Key *` comes from `keyNew`, `keyCopy`/`keyDup` or `keyBorrow`).
+
+## Implications
+


Implications should state which code needs to be changed.

Still missing.

markus2330 · 2021-07-14T19:11:25Z

doc/decisions/remove_elektra_malloc.md

@@ -0,0 +1,40 @@
+# Remove elektraMalloc() et al.


A related question is the documentation of the life-time of our objects. Here also improvements of the docu is needed.

doc/decisions/remove_elektra_malloc.md

markus2330 · 2021-07-14T19:14:02Z

doc/decisions/remove_elektra_malloc.md

+
+My main goal with removing elektraMalloc et al. is to improve developer friendliness, especially for newcomers. It is still annoying that I using e.g. strndup may be problematic, because it doesn't use elektraMalloc for allocations. But when I was new to Elektra (and I suspect many other felt the same), it was downright frustrating that you have to remember to use Elektra's special sauce instead of the standard stuff.
+
+The functions `elektraRealloc` and `elektraMemDup` (renamed from elektraStrNDup) can stay, because they are an improvement upon the libc.


This would be an decision, not a rationale.

markus2330 · 2021-07-14T19:33:11Z

Aliases are not mentioned?

lawli3t · 2021-07-18T17:01:03Z

I have added and edited the documents with the decision from our meeting. Feel free to comment and add to those documents in case I have forgotten something.

@markus2330 @kodebach @mpranj @tucek

doc/decisions/error_handling.md

doc/decisions/reference_counting.md

doc/decisions/remove_elektra_malloc.md

markus2330

Thank you, great work!

doc/decisions/functions_with_buffers.md

markus2330 · 2021-07-20T10:26:11Z

doc/decisions/iterators.md

+
+## Problem
+
+The internal iterator inside KeySets seems to cause more problems than it solves.


Maybe write a bit about the background why it was introduced and that this is not used anymore (like discussed in the meeting: to remember errorKey in kdbSet).

doc/decisions/iterators.md

markus2330 · 2021-07-20T10:28:58Z

doc/decisions/key_string_return_value.md

+
+## Constraints
+
+Because the return value of the function is const char\*, there are not so many possibilities for a change to this behaviour, as every possible return value - except for a NULL pointer - could just be the value of the string of the Key. Possible changes to this behaviour would be:


Actually we could also change the return value, so this is not really a constraint.

I think we can say "we don't want to change the return value", because that would make the function very inconvenient to use.

"return value" is an error from the original text. Probably changing the API is meant?

doc/decisions/key_string_return_value.md

markus2330 · 2021-07-20T10:41:59Z

doc/decisions/reference_counting.md

+
+## Decision
+
+- add second reference counter to Key


See #2202: probably it is simply a documentation issue and no second ref counter is needed

Honestly, what was the decision meeting for, if we immediately reverse one of the main decisions?

See #3949 (comment) for an idea on how documentation/API naming can make the second ref counter much easier to understand.

Honestly, what was the decision meeting for, if we immediately reverse one of the main decisions?

In some cases we also went into timeouts and agreed there would be a follow up.

See #3949 (comment) for an idea on how documentation/API naming can make the second ref counter much easier to understand.

I like the idea but this immediately raises the question why locking the value or meta-data does not get ref-counted.

raises the question why locking the value or meta-data does not get ref-counted.

To me the question is: Why is locking value/meta-data a thing at all? Is there a use-case for it?

AFAICT we only directly use KEY_FLAG_RO_VALUE and KEY_FLAG_RO_META inside keySetMeta. KEY_LOCK_VALUE and KEY_LOCK_META are not used at all.

For the metadata case we should actually check the key name in keySetRawValue and keyMeta. Otherwise it won't work with ksAppendKey (keyMeta (k), ...) or if you create a meta key via keySetName (k, "meta:/foo").

To me the question is: Why is locking value/meta-data a thing at all? Is there a use-case for it?

It is an invariant of a KeySet that its keys are ordered. The locking ensures this invariant.

KEY_LOCK_META

The idea was to use this for meta keys. But probably this is now useless, as meta keys can be recognized by the namespace.

Sorry, misread. I was thinking about if we can get rid of locking at all. And probably we can, we could simply disallow name changes with ref counter != initial value.

To answer your previous question: Locking of values was added for completeness. Other than read-only keys there is probably no usecase.

we could simply disallow name changes with ref counter != initial value.

Then you can't ever change the name from Java or another language with automatic garbage collection, where incrementing the ref counter is required.

As far as i understand:

keysets store keys in sorted by key names

a key's order within a key set is determined on insertion

the purpose of locking a key's name is to prevent (potentially) compromising the order of keys for keysets the key is contained in.

To better understand the issue:

what are the use cases for changing a key's name? why not making it immutable?

why is order within keysets so important? would it be sufficient to ensure order explicitly for use-cases requiring it?

what are the use cases for changing a key's name? why not making it immutable?

In short, to avoid doing some memory allocations. In my case I had a loop where a key was removed and a new one was added with a new basename. The same can be done with immutable keys, but it requires allocating new memory for N keys instead of reusing the old keys.

I was just confused that unlocking is currently not possible at all. When a key is added to a keyset the name is locked, but when it is removed from the keyset the name is not unlocked. To be consistent we should either have immutable keys or provide a way to unlock the keys again.

why is order within keysets so important?

It is required because we use binary search to find a key in the keyset. It is also a nice property and I don't see much reason to rework this, unless there are some huge benefits?

I think the Hashmap requires the order as well, since it only stores indices into the array. Also ksCut relies on the order of the KeySet and in general many things are easier or more efficient to implement, if there is a fixed order.

I was just confused that unlocking is currently not possible at all.

I had the same experience.

doc/decisions/reference_counting.md

markus2330 · 2021-07-20T10:48:17Z

doc/decisions/reference_counting.md

+- use `keyDel` to dispose of a `Key *` (whether the `Key *` comes from `keyNew`, `keyCopy`/`keyDup` or `keyBorrow`).
+
+## Implications
+


Still missing.

doc/decisions/iterators.md

markus2330 · 2021-09-16T15:27:21Z

How can we help you to get this merged?

lawli3t · 2021-09-16T17:08:45Z

Good question, I have resolved everything I could immediately yesterday. The following things are still open:

Stuff I can & will add soon:

Reworking Problem part of error_handling
Improvements to remove_elektra_malloc (restructuring Rationale, )
Improve problem part of Iterators

Stuff where I feel I don't have enough knowledge/overview to properly improve the existing documents:

Resolving the big discussion on the reference counting Decision part
Adding implications for reference counting

kodebach · 2021-09-16T21:52:08Z

I think reference counting was basically resolved with #3949 (comment). Additionally there will be second counter for the key name lock. However, I wouldn't call that reference counting. I would simply say the lock is shared via a counter.

With the decision to keep keyIncRef/keyDecRef, the only implications of the reference counting stuff are (AFAIK):

KeySets can be reference counted now. This is useful for bindings, and some (e.g. C++) may need updates.
Reference counts are now limited to UINT16_MAX - 1 instead of SIZE_MAX. This probably won't have any real impact, apart from a change in return type.

The implication of the counter for the name lock is simply that key names will be automatically unlocked again, when a key is removed from all keysets.

markus2330 · 2021-09-17T14:43:44Z

I think reference counting was basically resolved with #3949 (comment)

This still needs to be updated here in the decisions. 1-3 I agree, 4 see below, 5 we need to investigate if it actually solves the JNI problem.

Additionally there will be second counter for the key name lock. However, I wouldn't call that reference counting. I would simply say the lock is shared via a counter.

This is a separate issue #2202 where the current consensus is contrary: to not have a second ref counter just for being able to rename keys. Let us continue the discussion there.

This is useful for bindings, and some (e.g. C++) may need updates.

Yes, it needs an update. The C++ binding was very awkward: Key and KeySets had very different behavior when being copied or copy-constructed. It might be a quite big change though as most bindings wrap the C++ KeySet.

Reference counts are now limited to UINT16_MAX - 1 instead of SIZE_MAX. This probably won't have any real impact, apart from a change in return type.

Actually it is even an improvement as we get rid of platform-dependent behavior.

The implication of the counter for the name lock is simply that key names will be automatically unlocked again, when a key is removed from all keysets.

This is not an implication of anything said above. Please let us separate topics which are unrelated whenever possible. -> #2202

kodebach · 2021-09-17T14:55:16Z

Please let us separate topics which are unrelated whenever possible. -> #2202

I just mentioned it, because we talked about it in the meeting for this issue.

markus2330

In order to finish this, I think we should move still unclear decisions to "In Discussion". Then we are hopefully able to merge this soon.

markus2330 · 2021-11-28T07:38:28Z

doc/decisions/README.md

@@ -55,6 +55,12 @@ section here.

 - [Array for Warnings](warning_array.md)
 - [Array](array.md)
+- [Error Handling](error_handling.md)


Probably we should not put too much to decided but only the ones we are already sure about how to implement it.

markus2330 · 2021-11-28T08:50:47Z

doc/decisions/error_handling.md

+
+## Problem
+
+Currently, a lot of functions return the error value, regardless of error. These poses some problems, for instance authors of bindings, where it is hard to tell which error exactly occurred. A big improvement to this would be returning different error codes for different types of errors, in order to be able to provide better information in cases of errors. Some functions also make use of an `errorKey`.


Be more concrete. Afaik the only problems are:

name modifications which can be either invalid name or locking the key name

getting values of (non-)binary keys, see also Binary metadata vs flag #4194

markus2330 · 2021-11-28T08:51:29Z

doc/decisions/error_handling.md

+
+## Considered Alternatives
+
+- Change return types for many functions across the API


We could also fix the concrete instances of where multiple errors exist.

markus2330 · 2021-11-28T08:52:41Z

doc/decisions/error_handling.md

+
+## Rationale
+
+This simplifies and unifies error reporting across all functions. It also introduces


On the contrary: it would make it more complicated and if we are not very careful less unified.

markus2330 · 2021-11-28T08:55:06Z

doc/decisions/functions_with_buffers.md

+
+## Decision
+
+- Remove Functions:


markus2330 · 2021-11-28T08:56:55Z

doc/decisions/key_string_return_value.md

+## Constraints
+
+Because the return value of the function is const char\*, there are not so many possibilities for a change to this behavior, as every possible return value - except for a NULL pointer - could just be the value of the string of the Key. Possible changes to this behavior would be:
+


sentence stops in the middle?

markus2330 · 2021-11-28T08:57:54Z

doc/decisions/README.md

@@ -55,6 +55,12 @@ section here.

 - [Array for Warnings](warning_array.md)
 - [Array](array.md)
+- [Error Handling](error_handling.md)
+- [Iterators](iterators.md)
+- [keyString() return value](key_string_return_value.md)


As this depends on error_handling.md, probably we should also move it to "In Discussion".

markus2330 · 2021-11-28T08:58:16Z

doc/decisions/key_string_return_value.md

+## Implications
+
+## Related Decisions
+


error_handling.md should be here.

markus2330 · 2021-11-28T08:58:57Z

doc/decisions/reference_counting.md

+- use `keyDel` to dispose of a `Key *` (whether the `Key *` comes from `keyNew`, `keyCopy`/`keyDup` or `keyBorrow`).
+
+## Implications
+


Still missing.

Co-authored-by: kodebach <[email protected]>

Co-authored-by: Markus Raab <[email protected]>

markus2330 · 2021-12-09T14:53:01Z

Thank you so much! 💖

restyled-io bot mentioned this pull request Jul 4, 2021

Restyle doc: add decision docs for decision meeting mid july 2021 #3920

Closed

lawli3t self-assigned this Jul 4, 2021

lawli3t added documentation help wanted work in progress labels Jul 4, 2021

lawli3t changed the title ~~doc: add decision docs for decision meeting mid july 2021~~ doc: add decision docs for decision meeting 15.07.2021 Jul 4, 2021

markus2330 approved these changes Jul 14, 2021

View reviewed changes

lawli3t force-pushed the decision-meeting branch from 48f6abc to e9daa00 Compare July 18, 2021 16:59

restyled-io bot mentioned this pull request Jul 18, 2021

Restyle doc: add decision docs for decision meeting 15.07.2021 #3951

Closed

kodebach reviewed Jul 18, 2021

View reviewed changes

markus2330 approved these changes Jul 20, 2021

View reviewed changes

This was referenced Jul 20, 2021

Restyle doc: add decision docs for decision meeting 15.07.2021 #3952

Closed

Restyle doc: add decision docs for decision meeting 15.07.2021 #3958

Closed

lawli3t commented Jul 23, 2021

View reviewed changes

doc/decisions/iterators.md Show resolved Hide resolved

lawli3t force-pushed the decision-meeting branch from 156d88e to aabf544 Compare July 23, 2021 03:24

lawli3t force-pushed the decision-meeting branch from 87e357d to b5e1d11 Compare September 15, 2021 21:50

markus2330 suggested changes Nov 28, 2021

View reviewed changes

restyled-commits and others added 4 commits December 8, 2021 18:42

Restyled by prettier-markdown

8cbdf5c

doc: add links to new decisions to overview

e12835f

update decision docs with decisions from meeting

b00ace8

Restyled by prettier-markdown

1ba13d5

lawli3t and others added 11 commits December 8, 2021 18:42

add release notes

534ae44

Apply suggestions from code review

51ae3a2

Co-authored-by: kodebach <[email protected]>

fix spelling

d248c88

some minor clarifications for decisions

d0b9fb7

Restyled by prettier-markdown

a88afe3

some minor clarifications / fixes for decisions

e933563

Restyled by prettier-markdown

377730d

Apply suggestions from code review

fa75e44

Co-authored-by: Markus Raab <[email protected]>

doc: add additional functions scheduled for removal

f434c37

doc: improve documentation for reference_counting

775cdcd

move undecided issues to "In Progress"

90e8b10

lawli3t force-pushed the decision-meeting branch from b5e1d11 to 90e8b10 Compare December 8, 2021 17:44

markus2330 merged commit 43499ed into ElektraInitiative:master Dec 9, 2021

mpranj added this to the 0.9.9 milestone Mar 8, 2022


		## Problem

		Currently, a lot of functions return the error value, regardless of error. These poses some problems, for instance authors of bindings, where it is hard to tell which error exactly occured. A big improvement to this would be returning different error codes for different types of errors, in order to be able to provide better information in cases of errors. Some functions also make use of an `errorKey`. While this seems like a decent approach, to provide additional informations in cases of error, it is currently very badly documented. It is not really transparent for the users of the function, what kind of information is included in the `errorKey` and even which messages could possibly be returned.

		- use `keyDel` to dispose of a `Key ` (whether the `Key ` comes from `keyNew`, `keyCopy`/`keyDup` or `keyBorrow`).

		## Implications


		My main goal with removing elektraMalloc et al. is to improve developer friendliness, especially for newcomers. It is still annoying that I using e.g. strndup may be problematic, because it doesn't use elektraMalloc for allocations. But when I was new to Elektra (and I suspect many other felt the same), it was downright frustrating that you have to remember to use Elektra's special sauce instead of the standard stuff.

		The functions `elektraRealloc` and `elektraMemDup` (renamed from elektraStrNDup) can stay, because they are an improvement upon the libc.


		## Problem

		The internal iterator inside KeySets seems to cause more problems than it solves.


		## Constraints

		Because the return value of the function is const char\*, there are not so many possibilities for a change to this behaviour, as every possible return value - except for a NULL pointer - could just be the value of the string of the Key. Possible changes to this behaviour would be:


		## Considered Alternatives

		- Change return types for many functions across the API


		## Rationale

		This simplifies and unifies error reporting across all functions. It also introduces

doc: add decision docs for decision meeting 15.07.2021 #3919

doc: add decision docs for decision meeting 15.07.2021 #3919

Conversation

lawli3t commented Jul 4, 2021 • edited Loading

Basics

Checklist

Review

Labels

lawli3t commented Jul 4, 2021

kodebach commented Jul 4, 2021 • edited Loading

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 commented Jul 14, 2021

lawli3t commented Jul 18, 2021

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 commented Sep 16, 2021

lawli3t commented Sep 16, 2021

kodebach commented Sep 16, 2021

markus2330 commented Sep 17, 2021

kodebach commented Sep 17, 2021

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 commented Dec 9, 2021

lawli3t commented Jul 4, 2021 •

edited

Loading

kodebach commented Jul 4, 2021 •

edited

Loading