Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: spec fix #555

Merged
merged 4 commits into from
Apr 23, 2016
Merged

WIP: spec fix #555

merged 4 commits into from
Apr 23, 2016

Conversation

tom-wa
Copy link
Contributor

@tom-wa tom-wa commented Mar 7, 2016

crude hacked together code, just a possible approach

@tom-wa tom-wa changed the title fix #554 WIP: spec fix Mar 7, 2016
@tom-wa
Copy link
Contributor Author

tom-wa commented Mar 7, 2016

@markus2330 is this kind of approach to fix the problem with getting non-existing keys ok with you ?

@markus2330
Copy link
Contributor

It seems like you try to solve an ordering issue in an unappropriate way.
Certainly not every plugin is fine with getting executed twice.
Why not add an additional hook as discussed?

@tom-wa
Copy link
Contributor Author

tom-wa commented Mar 8, 2016

couldn't fine any other way to do it.
the keyset needed for global plugins (spec) only exists after elektraGetDoUpdate, but the backends plugins are all executed within elektraGetDotUpdate.
placement 6-9 need to be executed after the hook.
an other way to do it would be only running 1-5 within elektraGetDoUpdate, then executing the hook, and then running 6-9, but there is the possibility that some of those needs to be executed before the hook or independent from the hook.
this was the only approach i could find without doing any bigger changes to kdbGet

@tom-wa tom-wa force-pushed the fix_554 branch 3 times, most recently from 98b1b60 to fe294d0 Compare March 14, 2016 07:16
@@ -693,6 +726,15 @@ int kdbGet (KDB * handle, KeySet * ks, Key * parentKey)

ksRewind (ks);

if (elektraGetDoUpdate2 (split, ks, parentKey) == -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the problem is: this is called too late, you cannot call doUpdate after postprocessing of the keysets.

What about following logic:

elektraGetDoUpdate
hook MIDDLEGETSTORAGE
elektraGetDoUpdate2
elektraSplitGet // postprocessing
...
hook POSTGETSTORAGE

and spec will be called in MIDDLEGETSTORAGE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

elektraSplitMerge needs to be called before spec, and elektraSplitGet before elektraSplitMerge. So i guess i'd have to adjust elektraSplitGet but i'm not sure how to do the part with the syncbits

@tom-wa
Copy link
Contributor Author

tom-wa commented Apr 14, 2016

@markus2330 can you take a look at the currrent approach ?
it seems to work fine, but elektraSplitMerge is called twice. not sure how much bad the performance impact really is.

@@ -526,7 +527,39 @@ static int elektraGetDoUpdate (Split * split, Key * parentKey)
return 0;
}

static int elektraGetDoUpdate2 (Split * split, KeySet * ks, Key * parentKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like the duplication of the code, can you add the NR_OF_PLUGINS as parameter?

@markus2330
Copy link
Contributor

elektraSplitMerge should be quite cheap, but better to benchmark it. Actually, having a more efficient ksAppend is in TODO. (You can take advantage of that keysets are sorted when giving them together.) If it is expensive we could avoid it to be done twice if no such hook exists?

Doesn't it create a problem if a plugin removed some keys after position 6? (Because they would stay in the merged key set then).Don't you need to work on a duplicated keyset temporarily for the first elektraSplitMerge?

@markus2330
Copy link
Contributor

But looks better now and less invasive, but there are still code duplication and formatting issues. Also the PR is too big, there are many unrelated parts here.

@tom-wa tom-wa force-pushed the fix_554 branch 3 times, most recently from c2a5d27 to 75c95b2 Compare April 17, 2016 17:09
@tom-wa
Copy link
Contributor Author

tom-wa commented Apr 17, 2016

@markus2330 done.

@markus2330 markus2330 mentioned this pull request Apr 18, 2016
@markus2330 markus2330 merged commit 119cad6 into ElektraInitiative:master Apr 23, 2016
@markus2330
Copy link
Contributor

Thank you!

@tom-wa tom-wa mentioned this pull request Apr 24, 2016
@tom-wa tom-wa deleted the fix_554 branch January 30, 2017 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants