You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "alerman (via GitHub)" <gi...@apache.org> on 2024/04/17 18:29:57 UTC
[PR] Add visibility to grep searches [accumulo]
alerman opened a new pull request, #4468:
URL: https://github.com/apache/accumulo/pull/4468
Add visibility as one of the critera that the grep searches
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add visibility to grep searches [accumulo]
Posted by "alerman (via GitHub)" <gi...@apache.org>.
alerman commented on PR #4468:
URL: https://github.com/apache/accumulo/pull/4468#issuecomment-2074840925
@ctubbsii Thanks for the feedback. I have to say though that I disagree. While it may have been done as that isn't typically "searchable user data", I would argue you could also call this a bug as the grep command (and iterator) search every part of the key which is a byte array EXCEPT the visibility. Can you point me to documentation which specifies that the visibility is a special part of the key that should be treated differently in some cases? The data model page makes no such note.
On top of that, this PR came out of observing a user of accumulo frustrated as the grep they were running was not matching, when they expected that it would. Seems to me that the logical behavior is that it greps for everything, not that it excludes some parts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add visibility to grep searches [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #4468:
URL: https://github.com/apache/accumulo/pull/4468#issuecomment-2075536752
> I would argue you could also call this a bug as the grep command (and iterator) search every part of the key which is a byte array EXCEPT the visibility. Can you point me to documentation which specifies that the visibility is a special part of the key that should be treated differently in some cases? The data model page makes no such note.
Each piece of the key are inherently unique and special in the data model, but it's also flexible and you can certainly treat it like other fields, such as in your use case. My point wasn't that visibility must be treated special, but rather that it's also a valid use case if it is. The basis for my objection wasn't to preserve the existing use case, but to show that it's not necessarily a bug because both use cases are valid.
Treating both use cases as valid, then this change will break some valid existing workflows. The severity of that breakage could range anywhere from "harmless surprise", to "security-compromise", depending on the additional data returned. I think we can satisfy both use cases, while avoiding making it a breaking change.
> On top of that, this PR came out of observing a user of accumulo frustrated as the grep they were running was not matching, when they expected that it would. Seems to me that the logical behavior is that it greps for everything, not that it excludes some parts.
Is the main frustration the use of grep in the shell, or is it the GrepIterator itself? I'm okay changing the behavior in the shell, which primarily is intended to support user-interactive ad-hoc workflows anyway. My concern is changing the default behavior of the GrepIterator when used directly in table configuration or as a scan-time iterator.
A few things we could do to avoid breaking changes to the GrepIterator:
1. Create a new iterator with new behavior (this gives the possibility for deprecating the old one, if we want to only have one, and wish to signal the change in behavior), or
2. Add a matchOnVisibility option to existing iterator that is off by default (my preference).
For the shell, where we don't care about changing behavior as much:
1. Update the grep command in the shell to use the new iterator or new option and update the documentation (also, highlight the change in behavior in the release notes), and
2. Maybe also add an option to toggle between the behaviors in the shell (I don't think this is necessary... changing the behavior in the shell is less a concern for me than breaking direct uses of the GrepIterator).
Changing the behavior of the shell's grep command to make it more convenient can be done without changing the behavior of GrepIterator for existing workflows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add visibility to grep searches [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4468:
URL: https://github.com/apache/accumulo/pull/4468#issuecomment-2067209093
I think the description of the GrepCommand needs to be updated to reflect the change.
https://github.com/apache/accumulo/blob/714fdb7e1ac949eebb05fbe6d0fd7fe36c20d5f9/shell/src/main/java/org/apache/accumulo/shell/commands/GrepCommand.java#L125-L129
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add visibility to grep searches [accumulo]
Posted by "ivakegg (via GitHub)" <gi...@apache.org>.
ivakegg commented on PR #4468:
URL: https://github.com/apache/accumulo/pull/4468#issuecomment-2253223381
Changing the grep iterator to use an option for whether to match against the visibility is fine with me. I think the shell should default that option to true.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add visibility to grep searches [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #4468:
URL: https://github.com/apache/accumulo/pull/4468#issuecomment-2073544411
This is a change in behavior to an existing user-facing class, that will change the data returned. This was likely not done initially because visibilities are typically used as access control, not searchable user data. While there may be some applications for this, I don't think it's a typical use case.
This will very likely break existing applications. I think this needs to be reverted as-is. To protect existing use cases, an extra option can be added (off by default) to include visibilities in the matching.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add visibility to grep searches [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr merged PR #4468:
URL: https://github.com/apache/accumulo/pull/4468
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add visibility to grep searches [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #4468:
URL: https://github.com/apache/accumulo/pull/4468#issuecomment-2254973750
I pushed the changes to this in commit bdb8ce9725e007435db55b438f577065fafb1151 to add options, keeping the behavior of GrepCommand, but defaulting to the previous behavior for any current user of GrepIterator directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org