From 11d8e9773f959dc5c9d39b15d2ef830703bdefc9 Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Thu, 24 Nov 2022 17:54:59 +0100 Subject: [PATCH 01/13] decision: move to in discussion, add some details --- .../change_tracking.md | 39 ++++++++++++------- doc/decisions/2_in_progress/copy_on_write.md | 4 +- doc/news/_preparation_next_release.md | 2 +- 3 files changed, 29 insertions(+), 16 deletions(-) rename doc/decisions/{0_drafts => 1_in_discussion}/change_tracking.md (77%) diff --git a/doc/decisions/0_drafts/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md similarity index 77% rename from doc/decisions/0_drafts/change_tracking.md rename to doc/decisions/1_in_discussion/change_tracking.md index a3990cad943..aaac86d778a 100644 --- a/doc/decisions/0_drafts/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -43,28 +43,43 @@ Change tracking must: - not duplicate data for each plugin that wants change tracking - work with all allowed sequences of `kdbGet` and `kdbSet` as [per this decision](operation_sequences.md) +We only want to track changes that are done by the user, not changes that are done by plugins. +I.e. the scope of change tracking is on what happens _outside_ of `kdbGet` and `kdbSet`. + The library `libelektra-core` must be kept minimal. ## Assumptions - It is possible to do change tracking with reasonable memory and computation overhead -- It is possible to design a single change tracking API that is useful - for all existing and future plugins +- It is possible to design a single change tracking API that is useful for all existing and future plugins +- False positivies are okay + - this may happend when some keys may have been changed by the user, but have subsequentially been "unchanged" by a transformation plugin + Scenario: hypothetical plugin that converts all values to UPPERCASE. + - user gets key `system:/background` with value `RED` + - user changes it to `red` + - changetracking detects that value has been changed, because `RED` != `red` + - plugin changes value to UPPERCASE `RED` + - consumers of the changetracking API will now get a false positive if they query whether `system:/background` has been changed + +- False negatives are not okay + ## Considered Alternatives -### Alternative 1 - Per-parent key tracking within `libelektra-kdb` +### Alternative 1 - Tracking withn `libelektra-kdb` -Do the per-parent-key tracking within `libelektra-kdb`, within the `kdbGet` and `kdbSet` operations. +Do the tracking within `libelektra-kdb`, within the `kdbGet` and `kdbSet` operations. -Essentially, we'd have a 'giant' hashmap in the form of (parent key)->(KeySet) for all parent keys that were used for a `kdbGet` operation in the lifetime of the `kdb` instance. +Essentially, we'd have a 'giant' keyset for all keys that were used returned by a `kdbGet` operation in the lifetime of the `kdb` instance. We also need to update it on `kdbSet` so that a future `kdbSet` operation without a `kdbGet` will also work. -When change tracking is enabled, this one will have the most memory overhead, as we need to deep-dup every key we have read or stored. +This approach does not limit which sequences of `kdbGet` and `kdbSet` calls are valid. + +With Elektra's global copy-on-write approach, the memory overhead of this approach shouldn't be too concerning. -### Alternative 2 - Per-parent key tracking with meta keys +### Alternative 2 - Tracking with meta keys -Do the per-parent-key tracking within `libelektra-kdb`, but with meta keys. +Do the tracking within `libelektra-kdb`, but with meta keys. Essentially the same approach as above, but instead of deep-duping, we add the original value as a metakey to every key. Not yet clear how we handle changes to metadata then. @@ -124,15 +139,13 @@ Downsides of this approach: Use the `backendData->keys` for change tracking We already store which keys have been returned by `kdbGet` for each backend within KDB. -Currently, however, this is not a deep duplication, as we are returning the keys from `backendData->keys` directly. +Currently, however, this is not a deep copy, as we are returning the keys from `backendData->keys` directly. This means we can not detect changes to the values or metadata of keys right now. We can, however, rely on this for detecting removed and added keys in its current form. -If we don't want to deep-dup it, we'd need to do something different to detect which keys have been modified. -One possibility would be to add metadata within the `key` functions (e.g. `meta:/elektra/original`), but that would violate the `libelektra-core` must be minimal constraint. +As we now implement copy-on-write for all keys and keysets within Elektra, we can deep copy what we return in `kdbGet`. -There is already [another decision](../2_in_progress/copy_on_write.md) which discusses adding general copy-on-write semantics to Elektra. -We could use this together with `backendData->keys` deep-duped to do memory efficient change tracking. +Another problem with this approach is that `backendData->keys` get cleared every time `kdbGet` is called. ## Decision diff --git a/doc/decisions/2_in_progress/copy_on_write.md b/doc/decisions/2_in_progress/copy_on_write.md index bc5f49b6056..47ad0541899 100644 --- a/doc/decisions/2_in_progress/copy_on_write.md +++ b/doc/decisions/2_in_progress/copy_on_write.md @@ -7,7 +7,7 @@ Currently, there are many places within Elektra where keys and keysets are dupli Most of those copied keys are never modified, but are required to be detached from the lifetime of the original instance. We want to introduce an in-memory copy-on-write mechanism to lower our memory usage. -In the near future, Elektra will also gain facilities for [change tracking](../0_drafts/change_tracking.md) and session recording, both of which will potentially again duplicate keys. +In the near future, Elektra will also gain facilities for [change tracking](../1_in_discussion/change_tracking.md) and session recording, both of which will potentially again duplicate keys. There are also aspirations to create a new, simple [internal cache](../3_decided/internal_cache.md) that would also benefit from a copy-on-write mechanism. ## Constraints @@ -525,7 +525,7 @@ Implement the full-blown COW approach. ## Related Decisions -- [change tracking](../0_drafts/change_tracking.md) +- [change tracking](../1_in_discussion/change_tracking.md) - [internal cache](../3_decided/internal_cache.md) ## Notes diff --git a/doc/news/_preparation_next_release.md b/doc/news/_preparation_next_release.md index 50f233b6538..c74c240d7fd 100644 --- a/doc/news/_preparation_next_release.md +++ b/doc/news/_preparation_next_release.md @@ -492,7 +492,7 @@ This section keeps you up-to-date with the multi-language support provided by El - decided [decision process](https://www.libelektra.org/decisions/decision-process) _(Markus Raab)_ - draft for [man pages](../decisions/0_drafts/man_pages.md) _(Markus Raab)_ - <> -- Add decision for [change tracking](../decisions/0_drafts/change_tracking.md) _(Maximilian Irlinger @atmaxinger)_ +- Add decision for [change tracking](../decisions/1_in_discussion/change_tracking.md) _(Maximilian Irlinger @atmaxinger)_ - <> - Create [decision](../decisions/0_drafts/operation_sequences.md) for allowed and prohibited operation seqences _(Maximilian Irlinger @atmaxinger)_ - <> From 32e2c3cb2d549fffb84db177f63a32338b960b52 Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Thu, 24 Nov 2022 18:03:29 +0100 Subject: [PATCH 02/13] decision: fix styling --- doc/decisions/1_in_discussion/change_tracking.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index aaac86d778a..964fe93c666 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -53,17 +53,17 @@ The library `libelektra-core` must be kept minimal. - It is possible to do change tracking with reasonable memory and computation overhead - It is possible to design a single change tracking API that is useful for all existing and future plugins - False positivies are okay + - this may happend when some keys may have been changed by the user, but have subsequentially been "unchanged" by a transformation plugin Scenario: hypothetical plugin that converts all values to UPPERCASE. - - user gets key `system:/background` with value `RED` - - user changes it to `red` - - changetracking detects that value has been changed, because `RED` != `red` - - plugin changes value to UPPERCASE `RED` - - consumers of the changetracking API will now get a false positive if they query whether `system:/background` has been changed + - user gets key `system:/background` with value `RED` + - user changes it to `red` + - changetracking detects that value has been changed, because `RED` != `red` + - plugin changes value to UPPERCASE `RED` + - consumers of the changetracking API will now get a false positive if they query whether `system:/background` has been changed - False negatives are not okay - ## Considered Alternatives ### Alternative 1 - Tracking withn `libelektra-kdb` From 668938bba0b747f2f0ca69715ec5cba3533301c2 Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Thu, 24 Nov 2022 18:39:45 +0100 Subject: [PATCH 03/13] decision: apply suggestions by @markus2330 Co-authored-by: Markus Raab --- doc/decisions/1_in_discussion/change_tracking.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index 964fe93c666..5437989c9ac 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -66,11 +66,11 @@ The library `libelektra-core` must be kept minimal. ## Considered Alternatives -### Alternative 1 - Tracking withn `libelektra-kdb` +### Alternative 1 - Tracking within `libelektra-kdb` Do the tracking within `libelektra-kdb`, within the `kdbGet` and `kdbSet` operations. -Essentially, we'd have a 'giant' keyset for all keys that were used returned by a `kdbGet` operation in the lifetime of the `kdb` instance. +Essentially, we'd have an internal cache for all keys that were used returned by a `kdbGet` operation in the lifetime of the `kdb` instance. We also need to update it on `kdbSet` so that a future `kdbSet` operation without a `kdbGet` will also work. This approach does not limit which sequences of `kdbGet` and `kdbSet` calls are valid. From 4a790919ef427cfe1ca9894ab86f736ec18484f1 Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Fri, 25 Nov 2022 22:01:01 +0100 Subject: [PATCH 04/13] decision: apply suggestions by @kodebach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com> --- doc/decisions/1_in_discussion/change_tracking.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index 5437989c9ac..18c7dc8e137 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -139,13 +139,13 @@ Downsides of this approach: Use the `backendData->keys` for change tracking We already store which keys have been returned by `kdbGet` for each backend within KDB. -Currently, however, this is not a deep copy, as we are returning the keys from `backendData->keys` directly. +Currently, however, this is not a deep copy, as we are returning the internally stored `Key` instances directly. This means we can not detect changes to the values or metadata of keys right now. We can, however, rely on this for detecting removed and added keys in its current form. As we now implement copy-on-write for all keys and keysets within Elektra, we can deep copy what we return in `kdbGet`. -Another problem with this approach is that `backendData->keys` get cleared every time `kdbGet` is called. +Another problem with this approach is that the internally stored keys are recreated as new instances every time `kdbGet` is called. ## Decision From c8a34d6f518b5ebbf1a3e16d614f12daf53545c3 Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Tue, 29 Nov 2022 00:48:30 +0100 Subject: [PATCH 05/13] decision: apply suggestions by @kodebach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com> --- doc/decisions/1_in_discussion/change_tracking.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index 18c7dc8e137..ff8f3065470 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -55,11 +55,12 @@ The library `libelektra-core` must be kept minimal. - False positivies are okay - this may happend when some keys may have been changed by the user, but have subsequentially been "unchanged" by a transformation plugin - Scenario: hypothetical plugin that converts all values to UPPERCASE. - - user gets key `system:/background` with value `RED` - - user changes it to `red` - - changetracking detects that value has been changed, because `RED` != `red` - - plugin changes value to UPPERCASE `RED` + Scenario: plugin that converts `false`<->`0` and `true`<->`1` + - `system:/background` is stored with value `false` + - user gets key `system:/background` with value `0` (after conversion by plugin) + - user changes it to `false` + - changetracking detects that value has been changed, because `false` != `0` + - plugin changes value from `false` to `0` - consumers of the changetracking API will now get a false positive if they query whether `system:/background` has been changed - False negatives are not okay From cf54005363a74845c249eb5629770e5c8690f06a Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Sat, 3 Dec 2022 19:08:15 +0100 Subject: [PATCH 06/13] decision: update links --- doc/decisions/0_drafts/operation_sequences.md | 4 ++-- doc/decisions/1_in_discussion/change_tracking.md | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/decisions/0_drafts/operation_sequences.md b/doc/decisions/0_drafts/operation_sequences.md index 78cd7d4238e..51cf1f34164 100644 --- a/doc/decisions/0_drafts/operation_sequences.md +++ b/doc/decisions/0_drafts/operation_sequences.md @@ -227,7 +227,7 @@ As can be seen, the change tracking within the `dbus` and `logchange` plugins wr This way, we have only one such algorithm in a central place, and plugin authors don't have to think about the sequences their plugins are called by developers. This approach can also be paired with [COW semantics](../3_decided/internal_cache.md), so that memory toll will be kept low. - A separate [decision for change tracking](change_tracking.md) is currently in progress. + A separate [decision for change tracking](../1_in_discussion/change_tracking.md) is currently in progress. Should we observe this problem with use cases other than change tracking, we can provide general frameworks for those too. @@ -243,7 +243,7 @@ As can be seen, the change tracking within the `dbus` and `logchange` plugins wr ## Related Decisions -- [Change Tracking](change_tracking.md) +- [Change Tracking](../1_in_discussion/change_tracking.md) - [Internal KeySet Cache](../3_decided/internal_cache.md) - []() diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index ff8f3065470..b832a5b1f48 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -13,7 +13,7 @@ These competing change tracking implementations create multiple problems: 1. pointless waste of resources, as data is duplicated in each plugin, 2. multiplication of code, generating a maintenance burden. 3. various subtle differences in change tracking behavior, e.g., `kdbSet` might write a config file but notification is not sent out -4. the current approach to change tracking in plugins is fragile, which is outlined in [a separate decision about valid kdbGet/kdbSet sequences](operation_sequences.md). +4. the current approach to change tracking in plugins is fragile, which is outlined in [a separate decision about valid kdbGet/kdbSet sequences](../0_drafts/operation_sequences.md). For `KeySet` we need to track which of the keys: @@ -41,7 +41,7 @@ Change tracking must: - if overhead is not negligible: only do tracking as required, i.e., a plugin specifically requests it - have negligible overhead if disabled - not duplicate data for each plugin that wants change tracking -- work with all allowed sequences of `kdbGet` and `kdbSet` as [per this decision](operation_sequences.md) +- work with all allowed sequences of `kdbGet` and `kdbSet` as [per this decision](../0_drafts/operation_sequences.md) We only want to track changes that are done by the user, not changes that are done by plugins. I.e. the scope of change tracking is on what happens _outside_ of `kdbGet` and `kdbSet`. @@ -160,7 +160,7 @@ Another problem with this approach is that the internally stored keys are recrea ## Related Decisions -- [Valid kdbGet/kdbSet sequences](operation_sequences.md) from [#4574](https://github.com/ElektraInitiative/libelektra/pull/4574). +- [Valid kdbGet/kdbSet sequences](../0_drafts/operation_sequences.md) from [#4574](https://github.com/ElektraInitiative/libelektra/pull/4574). - [Copy On Write](../2_in_progress/copy_on_write.md) ## Notes From c760eb6d6984ab75660c3791e35138f7b484b3d6 Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Fri, 16 Dec 2022 11:07:51 +0100 Subject: [PATCH 07/13] doc: clarify false negatives --- doc/decisions/1_in_discussion/change_tracking.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index b832a5b1f48..bbbb94938f3 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -63,7 +63,7 @@ The library `libelektra-core` must be kept minimal. - plugin changes value from `false` to `0` - consumers of the changetracking API will now get a false positive if they query whether `system:/background` has been changed -- False negatives are not okay +- False negatives (missed changes) are not okay ## Considered Alternatives From d423e669ed94a09fb11fb103f135efdcddf7c967 Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Mon, 23 Jan 2023 17:13:02 +0100 Subject: [PATCH 08/13] decision: restructure --- .../1_in_discussion/change_tracking.md | 107 ++++++++++-------- 1 file changed, 59 insertions(+), 48 deletions(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index bbbb94938f3..ce3ef7b8de6 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -65,60 +65,38 @@ The library `libelektra-core` must be kept minimal. - False negatives (missed changes) are not okay -## Considered Alternatives +## Solutions - Storage -### Alternative 1 - Tracking within `libelektra-kdb` +### Utilize already existing `backendData->keys` -Do the tracking within `libelektra-kdb`, within the `kdbGet` and `kdbSet` operations. - -Essentially, we'd have an internal cache for all keys that were used returned by a `kdbGet` operation in the lifetime of the `kdb` instance. -We also need to update it on `kdbSet` so that a future `kdbSet` operation without a `kdbGet` will also work. +We already store which keys have been returned by `kdbGet` for each backend within KDB. +Currently, those are shallow copies. +To be useful for changetracking, we need to perform deep copies instead. +As keys and keysets are already utilizing copy-on-write, this would not add too much memory overhead. -This approach does not limit which sequences of `kdbGet` and `kdbSet` calls are valid. +A problem with this approach is that the internally stored keys are recreated as new instances every time `kdbGet` is called. -With Elektra's global copy-on-write approach, the memory overhead of this approach shouldn't be too concerning. +### Combine with internal cache -### Alternative 2 - Tracking with meta keys +We already decided that we want to have an internal deep-duped keysets of all the keys we returned. +See [internal cache decision](../3_decided/internal_cache.md). -Do the tracking within `libelektra-kdb`, but with meta keys. +The difference to `backendData->keys` is that this cache is not recreated each time `kdbGet` is called. -Essentially the same approach as above, but instead of deep-duping, we add the original value -as a metakey to every key. Not yet clear how we handle changes to metadata then. +### Have a seperate storage for changetracking -### Alternative 3 - Change tracking within a separate plugin +Have a global keyset with deep-duped keys that is purely used for changetracking and nothing else. -Outsource the change tracking into a separate plugin. +### Store changes as meta keys -Essentially the same as (1), just that it is implemented within a plugin and not `libelektra-kdb`. -This will be a hook plugin, and will be called within `kdbGet` and `kdbSet` accordingly. -It will also need to export a hook-method to get the changeset. +When something changes for the first time, store the original value as a metakey to every key. +Not yet clear how we handle changes to metadata then. +Also not really possible for keysets. -The following hooks will be needed: - -- `tracking/get`: wil be called at the end of `kdbGet`, directly before the result is returned. -- `tracking/set/preliminary`: will be called at the beginning of `kdbSet` and after every step/phase in `kdbSet`. - We need this to have change tracking information as soon as possible, so that plugins in any step of the process can use this information. - Also, plugins in every step of the process might change data, so it is important to call it after every step. -- `tracking/set/final`: will be called in the `post-commit` phase, after all changes have been written to disk, but before the `notification/send` hook. - Represents the final, real changes to the KDB. -- `tracking/changeset`: compute the changeset for the requested parent key and return it. - -### Alternative 4 - Copy-on-write change tracking within `libelektra-core` +### Implement changetracking as a mechanismn on the `Key` and `KeySet` datastructures The idea here is that we extend the `KeySet` and `Key` structs with additional fields. -We need to extend `KeySet` with the following info: - -- What keys have been removed -- What keys have been added -- Whether tracking is enabled for this KeySet - -We need to extend `Key` with the following info: - -- Original value of the key -- Size of the original value (for binary keys) -- Whether tracking is enabled for this key - The tracking itself would be done within the `ks*` and `key*` methods, after checking if it is enabled. It would also transparently work for metadata, as metadata itself is implemented as a keyset with keys. @@ -135,18 +113,51 @@ Downsides of this approach: - Another downside here is that it is not so easy to determine what the "original" value is. Some part of `libelektra-kdb` would need to mark the keys as original (after transformations etc.) -### Alternative 5 - Use `backendData->keys` for change tracking +## Solutions - Implementation -Use the `backendData->keys` for change tracking +### Implement directly within `libelektra-kdb` -We already store which keys have been returned by `kdbGet` for each backend within KDB. -Currently, however, this is not a deep copy, as we are returning the internally stored `Key` instances directly. -This means we can not detect changes to the values or metadata of keys right now. -We can, however, rely on this for detecting removed and added keys in its current form. +Implement all the logic for changetracking directly within `libelektra-kdb`. + +### Implement as a seperate plugin + +Implement changetracking as a hooks plugin that will be called within `kdbGet` and `kdbSet` accordingly. + +The following hooks will be needed: + +- `tracking/get`: will be called at the end of `kdbGet`, directly before the result is returned. +- `tracking/set/preliminary`: will be called at the beginning of `kdbSet` and after every step/phase in `kdbSet`. + We need this to have change tracking information as soon as possible, so that plugins in any step of the process can use this information. + Also, plugins in every step of the process might change data, so it is important to call it after every step. +- `tracking/set/final`: will be called in the `post-commit` phase, after all changes have been written to disk, but before the `notification/send` hook. + Represents the final, real changes to the KDB. +- `tracking/changeset`: compute the changeset for the requested parent key and return it. + +## Solutions - Query + +### Provide an API within `libelektra-kdb` + +The API should be useable both by plugins and applications utilizing ELektra. +The API may look something like this: + +```c +bool elektraChangeTrackingIsEnabled (KDB * kdb); +ChangeTrackingContext * elektraChangeTrackingGetContext (KDB * kdb, Key * parentKey); + +KeySet * elektraChangeTrackingGetAddedKeys (ChangeTrackingContext * context); +KeySet * elektraChangeTrackingGetRemovedKeys (ChangeTrackingContext * context); +KeySet * elektraChangeTrackingGetModifiedKeys (ChangeTrackingContext * context); + +bool elektraChangeTrackingValueChanged (ChangeTrackingContext * context, Key * key); +bool elektraChangeTrackingMetaChanged (ChangeTrackingContext * context, Key * key); -As we now implement copy-on-write for all keys and keysets within Elektra, we can deep copy what we return in `kdbGet`. +KeySet * elektraChangeTrackingGetAddedMetaKeys (ChangeTrackingContext * context, Key * key); +KeySet * elektraChangeTrackingGetRemovedMetaKeys (ChangeTrackingContext * context, Key * key); +KeySet * elektraChangeTrackingGetModifiedMetaKeys (ChangeTrackingContext * context, Key * key); -Another problem with this approach is that the internally stored keys are recreated as new instances every time `kdbGet` is called. +Key * elektraChangeTrackingGetOriginalKey (ChangeTrackingContext * context, Key * key); +const Key * elektraChangeTrackingGetOriginalMetaKey (ChangeTrackingContext * context, Key * key, const char * metaName); +``` ## Decision From 77bf115aed84a0c7266109378ec0c32b3c57740d Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Wed, 25 Jan 2023 12:51:40 +0100 Subject: [PATCH 09/13] decision: apply suggestions by @kodebach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com> --- doc/decisions/1_in_discussion/change_tracking.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index ce3ef7b8de6..b3459f3f6df 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -54,7 +54,7 @@ The library `libelektra-core` must be kept minimal. - It is possible to design a single change tracking API that is useful for all existing and future plugins - False positivies are okay - - this may happend when some keys may have been changed by the user, but have subsequentially been "unchanged" by a transformation plugin + - this may happend when some keys have been changed by the user, but have subsequentially been "unchanged" by a transformation plugin Scenario: plugin that converts `false`<->`0` and `true`<->`1` - `system:/background` is stored with value `false` - user gets key `system:/background` with value `0` (after conversion by plugin) @@ -62,6 +62,8 @@ The library `libelektra-core` must be kept minimal. - changetracking detects that value has been changed, because `false` != `0` - plugin changes value from `false` to `0` - consumers of the changetracking API will now get a false positive if they query whether `system:/background` has been changed + - We assume consumers of the changetracking API have access to both the previous and the new value of a changed key. + Therefore, they could detect these false positive cases, if the need to. - False negatives (missed changes) are not okay From 46b0aabd82765aeb9668a96a54745bd05c203d89 Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Fri, 27 Jan 2023 20:17:37 +0100 Subject: [PATCH 10/13] decision: some clarifications on plugin --- doc/decisions/1_in_discussion/change_tracking.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index b3459f3f6df..23aaf879b3c 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -128,13 +128,12 @@ Implement changetracking as a hooks plugin that will be called within `kdbGet` a The following hooks will be needed: - `tracking/get`: will be called at the end of `kdbGet`, directly before the result is returned. -- `tracking/set/preliminary`: will be called at the beginning of `kdbSet` and after every step/phase in `kdbSet`. - We need this to have change tracking information as soon as possible, so that plugins in any step of the process can use this information. - Also, plugins in every step of the process might change data, so it is important to call it after every step. -- `tracking/set/final`: will be called in the `post-commit` phase, after all changes have been written to disk, but before the `notification/send` hook. - Represents the final, real changes to the KDB. +- `tracking/set`: will be called at the beginning of `kdbSet`. - `tracking/changeset`: compute the changeset for the requested parent key and return it. +As every hook plugin can define its own contract, in theory all storage forms mentioned in the previous chapter should be possible to implement. +We could just point the plugin to `backendsData->keys` or the internal cache if we go down that route. + ## Solutions - Query ### Provide an API within `libelektra-kdb` From 79a5541a15eaa928402085c2f8b265550bfcad52 Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Fri, 27 Jan 2023 23:31:08 +0100 Subject: [PATCH 11/13] run checks From ebd9bd11c856b00f06e276f39abf4126afb9e0e6 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 27 Jan 2023 22:31:23 +0000 Subject: [PATCH 12/13] Restyled by prettier-markdown --- doc/decisions/1_in_discussion/change_tracking.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index 23aaf879b3c..efed8872c51 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -91,7 +91,7 @@ Have a global keyset with deep-duped keys that is purely used for changetracking ### Store changes as meta keys -When something changes for the first time, store the original value as a metakey to every key. +When something changes for the first time, store the original value as a metakey to every key. Not yet clear how we handle changes to metadata then. Also not really possible for keysets. From 534d00d7ad7224084fe6d12ba61c2c62743285ca Mon Sep 17 00:00:00 2001 From: Maximilian Irlinger Date: Thu, 2 Feb 2023 20:37:54 +0100 Subject: [PATCH 13/13] decision: clarifications for query --- .../1_in_discussion/change_tracking.md | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/doc/decisions/1_in_discussion/change_tracking.md b/doc/decisions/1_in_discussion/change_tracking.md index efed8872c51..7317b85b350 100644 --- a/doc/decisions/1_in_discussion/change_tracking.md +++ b/doc/decisions/1_in_discussion/change_tracking.md @@ -139,6 +139,7 @@ We could just point the plugin to `backendsData->keys` or the internal cache if ### Provide an API within `libelektra-kdb` The API should be useable both by plugins and applications utilizing ELektra. +It does not matter whether the changetracking is implemented as part of `libelektra-kdb` or as a seperate plugin. The API may look something like this: ```c @@ -147,19 +148,30 @@ ChangeTrackingContext * elektraChangeTrackingGetContext (KDB * kdb, Key * parent KeySet * elektraChangeTrackingGetAddedKeys (ChangeTrackingContext * context); KeySet * elektraChangeTrackingGetRemovedKeys (ChangeTrackingContext * context); -KeySet * elektraChangeTrackingGetModifiedKeys (ChangeTrackingContext * context); +KeySet * elektraChangeTrackingGetModifiedKeys (ChangeTrackingContext * context); // Returns old keys (pre-modification) bool elektraChangeTrackingValueChanged (ChangeTrackingContext * context, Key * key); bool elektraChangeTrackingMetaChanged (ChangeTrackingContext * context, Key * key); KeySet * elektraChangeTrackingGetAddedMetaKeys (ChangeTrackingContext * context, Key * key); KeySet * elektraChangeTrackingGetRemovedMetaKeys (ChangeTrackingContext * context, Key * key); -KeySet * elektraChangeTrackingGetModifiedMetaKeys (ChangeTrackingContext * context, Key * key); - -Key * elektraChangeTrackingGetOriginalKey (ChangeTrackingContext * context, Key * key); -const Key * elektraChangeTrackingGetOriginalMetaKey (ChangeTrackingContext * context, Key * key, const char * metaName); +KeySet * elektraChangeTrackingGetModifiedMetaKeys (ChangeTrackingContext * context, Key * key); // Returns old meta keys (pre-modification) ``` +### Provide query methods as part of a seperat plugin + +This solution only makes sense if changetrackig is implemented as part of a seperate plugin. +It will be a bit challenging to use for applications, as it would require that applications have access to the plugin contracts. + +The changetracking plugin needs to export at least functions for the following things in its contract: + +- Get added keys +- Get removed keys +- Get modified keys +- Get added meta keys for a key +- Get removed meta keys for a key +- Get modified meta keys for a key + ## Decision ## Rationale