Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

kdb disable cascading writes #3742

Closed
6 of 7 tasks
Tracked by #4431
markus2330 opened this issue Apr 1, 2021 · 12 comments
Closed
6 of 7 tasks
Tracked by #4431

kdb disable cascading writes #3742

markus2330 opened this issue Apr 1, 2021 · 12 comments
Assignees
Labels
Milestone

Comments

@markus2330
Copy link
Contributor

markus2330 commented Apr 1, 2021

As @dev2718 found out, it is best to disable cascading writes with an exception if the namespace is explicitly given. So disable cascading writes in:

  • kdb set (in ambigious cases)
  • kdb meta-set (in ambigious cases if they exist)
  • kdb import
  • kdb editor
  • kdb export (which is actually an cascading read but it doesn't make sense if kdb import does not work)
  • kdb mv
  • kdb rm
@kodebach
Copy link
Member

kodebach commented Apr 1, 2021

  • kdb editor

Has various problems anyway, but yes cascading keys definitely don't make sense here.

  • kdb export (which is actually an cascading read but it doesn't make sense if kdb import does not work)

Cascading export doesn't make sense anyway. Plugins should only use relative names when writing keys. But how would that work, if the keys are of different namespaces and therefore don't have a common parent.

  • kdb rm

I would keep it here, but require an additional flag parameter. Similar to how the Unix rm asks you to use -r when you pass a directory.

Similarly it could work for kdb mv. But you would have to use cascading keys for source and destination and all keys are only moved within their own namespaces, i.e. only the path relative to the namespace root is changed.


kdb set, kdb meta-set and kdb import should use a cascading key in their kdbGet and kdbSet calls to ensure spec:/ is properly handled. On the command-line they should always require a specific namespace. Whether the namespace is part of the key name or given as a separate argument is up for debate.

@dev2718
Copy link
Contributor

dev2718 commented Apr 2, 2021

Thanks for opening the issue.

I still have a few questions:

kdb set (in ambigious cases): How exactly do you define "ambiguous" here?

  1. == cascading lookup failed?
  2. == more than one key with the same name exist globally?
  3. ... something else?

Furthermore, does a successful lookup imply that we do a cascading write anyways or cascading writes are to be disabled in general?

kdb meta-set (in ambigious cases if they exist)
see above; furthermore: does "if they exist" refer to the cases or the keys?

Moreover, I second the questions from @kodebach.

Thanks in advance!

@kodebach
Copy link
Member

kodebach commented Apr 2, 2021

Moreover, I second the questions from @kodebach.

I didn't really have any questions... I made some suggestions.

But I may be able to answer your questions. I don't think we really need to clarify what "ambiguous" and "if they exist" mean. Like I said, kdb set and kdb meta-set should always expect a namespace on the command-line (in whatever form), but always use cascading keys in their kdbGet() and kdbSet() calls. This always applies, no matter if the case is "ambiguous" or "if they exist".

@markus2330
Copy link
Contributor Author

How exactly do you define "ambiguous" here?

That a lookup afterwards would not return the key we add. #401

Furthermore, does a successful lookup imply that we do a cascading write anyways or cascading writes are to be disabled in general?

Such questions are to be decided by you via the analysis you do. I tend towards more automatism (if no namespace given, simply use the user: namespace or system: if called by root but abort in ambiguous cases), @kodebach to less automatism (namespace must be there explicitly). I am not opposed to @kodebach's suggestion, this guessing of namespaces is maybe not the best idea (and I simply got used to it).

Anyway, we agree that ambiguous cases should be warned, do we?

@kodebach
Copy link
Member

kodebach commented Apr 3, 2021

if no namespace given, simply use the user: namespace or system: if called by root but abort in ambiguous cases

I don't really like the dependency on "user is root". What about no namespace always means user:/? (Note: there is a user:/ namespace for the root user, even if it probably has almost no use)

Anyway, we agree that ambiguous cases should be warned, do we?

I'd say there should either be no ambiguous cases, or ambiguous cases should result in an error (i.e. do nothing an request clarification).

@markus2330
Copy link
Contributor Author

The code is here:

if (getuid () == 0 || geteuid () == 0)

I don't know if user: is a better default. For meta-set spec: might be a default worth considering. But then again we would have a "surprising" magic.

@kodebach
Copy link
Member

kodebach commented Apr 3, 2021

For meta-set spec: might be a default worth considering. But then again we would have a "surprising" magic.

The fact that kdb meta-set defaults to spec:/ trips me up every time. I really don't like it.

@markus2330
Copy link
Contributor Author

Probably we should really always specify the namespace.

@stale
Copy link

stale bot commented Apr 6, 2022

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 💖

@stale stale bot added stale and removed stale labels Apr 6, 2022
@markus2330 markus2330 changed the title disable cascading writes kdb disable cascading writes Aug 6, 2022
@markus2330
Copy link
Contributor Author

@atmaxinger here is the question if you rewrite the tooling so that this bug gets fixed.

@markus2330 markus2330 assigned hannes99 and unassigned atmaxinger Aug 8, 2022
hannes99 added a commit that referenced this issue May 31, 2023
... and (partial) fix #3742
hannes99 added a commit that referenced this issue May 31, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue May 31, 2023
... and (partial) fix #3742
hannes99 added a commit that referenced this issue Jun 1, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 1, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 1, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 1, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 1, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 2, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 2, 2023
... and (partial) fix #3742
hannes99 added a commit that referenced this issue Jun 8, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 8, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 8, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 8, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 9, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 9, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 10, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 11, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 13, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 14, 2023
hannes99 added a commit that referenced this issue Jun 14, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jun 20, 2023
... and (partial) fix #3742
hannes99 added a commit that referenced this issue Jul 5, 2023
... and (partial) fix #3742, fix #4028, fix #1164
hannes99 added a commit that referenced this issue Jul 11, 2023
... and (partial) fix #3742
Copy link

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 💖

@github-actions github-actions bot added the stale label Apr 20, 2024
Copy link

github-actions bot commented May 4, 2024

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 💖

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants