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

kdb export system #3299

Open
kodebach opened this issue Nov 28, 2019 · 7 comments
Labels

Comments

@kodebach
Copy link
Contributor

@kodebach kodebach commented Nov 28, 2019

In #3115 I noticed that kdb export system (kdb export system:/ in the PR), also exports everything in system/elektra/modules. Is this intended?

For the specific circumstance see #3115 (comment)

@kodebach kodebach added the question label Nov 28, 2019
@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 28, 2019

Of course, why not? It also exports version and other stuff not useful for import. You can add --without-elektra to avoid exporting these parts.

For the shell-recorder I would also expect that only the given part is backed up and restored.

@kodebach

This comment has been minimized.

Copy link
Contributor Author

@kodebach kodebach commented Nov 28, 2019

Of course, why not?

Does kdbSet remove them somehow? It seems wrong to serialize these keys, when they are hard coded via a special part of plugin get functions...

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 29, 2019

For the version information kdbSet checks if they are identical and ignores kdbSet if they are. Otherwise an error gets generated.

For modules (iirc) there is no kdbSet, so the keys to set are simply ignored. Iirc the "constants" plugin has the same behavior It is usually mounted system/info/elektra so not in system/elektra. Maybe we should change that to make --without-elektra more useful?

But anyway: Both behaviors (ignoring and checking for identical keys) should not harm an import.

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 29, 2019

So for the current situation of the quite unstable Elektra it makes perfect sense: imports of system/elektra are rejected unless the version is identical. To avoid this error, --without-elektra can be passed.

After 1.0 we can actually be more relaxed and accept imports from Elektra versions 1.0 or greater. (@mpranj what do you think?)

The text needs to be adapted then, currently it is:

kdb import system < sys
Sorry, module kdb issued the error C01320:
Interface: Read only plugin, 'kdbSet' not supported but the key system/elektra/version/constants/KDB_VERSION_MICRO (expected system/elektra/version/constants/KDB_VERSION_MICRO) was tried to be modified to '1' (expected '2')

Anyway, your question gives insight about how we can improve the documentation of --without-elektra. The idea of --without-elektra in context of import/export is that either:

  1. all the internas of Elektra are exported but then we also have the version check to not mess up Elektra.
  2. you do not import/export internas of Elektra, so version of Elektra does not matter (only the version of the storage plugin does)
@mpranj

This comment has been minimized.

Copy link
Member

@mpranj mpranj commented Nov 29, 2019

After 1.0 we can actually be more relaxed

I agree with the suggestions.

As @kodebach mentioned, it is somehow weird that this appeared just now in #3115 and did not appear before. While adding --without-elektra probably should have been there in the first place for backup-and-restore, it still seems that the PR changed some behavior.

@kodebach did you activate cache again for those tests? I'd like to wait for the rest of the PR to be done so I can fix mmapstorage and cache. I have seen such/similar errors (Postcondition of backend was violated: drop key [...] not belonging to [...]) while the cache was not implemented correctly.

@kodebach

This comment has been minimized.

Copy link
Contributor Author

@kodebach kodebach commented Nov 29, 2019

did you activate cache again for those tests?

Yes, cache is fully enabled.

so I can fix mmapstorage and cache

mmapstorage works already. Turns out I had a key instead of ukey somewhere.

The cache also seems to work, apart from the problem with the global unmount test. Last I checked, the error didn't happen when cache isn't compiled.

I did change parts of kdbGet, elektraCacheGet and related functions. It's likely I broke something, but since the things I changed probably have to be changed again after #2969, I don't think you need to take a look right now.

@mpranj

This comment has been minimized.

Copy link
Member

@mpranj mpranj commented Nov 29, 2019

Ok. I suggest if the cache+mmapstorage causes the problems you simply keep it deactivated until the very end and we activate adapt it then.

The PR is a quite fundamental change so it will be much easier if you don't get stuck because of some broken cache behavior. When it's basically ready simply activate cache again and ping me so I can take a look and fix the remainders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.