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

ACCUMULO-1730 improvements #2

Closed
wants to merge 5 commits into from
Closed

ACCUMULO-1730 improvements #2

wants to merge 5 commits into from

Conversation

jstoneham
Copy link

Cherry-pick @ericnewton's commit since it never made it to the backport branch. Correct start and end indices for AND and OR nodes.

@ctubbsii
Copy link
Member

ctubbsii commented Oct 9, 2013

It's fine to cherry-pick to backport to 1.4.5 as you did in pull request #1, but we want to keep the full 1.4 history as a part of 1.5, and the full 1.5 history as part of master, etc. So, once you've cherry-picked back to 1.4, you should then merge these changes to the 1.5 branch, rather than cherry pick them onto 1.5 again. This eliminates duplicate commits and makes history nice and clean.

This pull request #2, is not a merge forward of the commits from #1, and is unusable if we follow those conventions. It'd be better to merge forward, resolve the conflicts, and submit that as a pull request for 1.5, and then similarly initiate a pull request to master after merging forward to master (effectively a no-op since the commits were cherry-picked from master, but it preserves a common history).

That said, I'm not sure this fix warrants backporting, as it's not a bugfix, but introduces potential bugs in important code that that we'd hate to break. The additional maintenance and testing costs are probably not worth the risk.

Additional discussion can be found at https://issues.apache.org/jira/browse/ACCUMULO-1730

@jstoneham
Copy link
Author

Hope this works better for 1.5.

If you decide not to backport to 1.4 I can redo this one again...

@ctubbsii
Copy link
Member

Out of curiosity, why is this needed? I'm really hesitant to backport this change, since it's not really a bug, and because of the risks.

@jstoneham
Copy link
Author

I was writing a MapReduce job to re-normalize the visibility strings in our database because the format we want to use has changed. This is easy enough on most visibility strings - parse them the old way, then re-write them out the new way. The issue I had was dealing with complex visibility strings with top-level ORs, such as (A&B)|(C&(D|E)). I wanted to use the parse tree to find the substrings of the top-level visibilities, since we can't safely just split on the | character. But the node offsets were wrong.

I understand the risk of the backport, and making a call on whether or not to add it the 1.4 baseline based on that. I'm still not sure why it doesn't "count as a bug", though.

@jstoneham jstoneham closed this Oct 15, 2013
asfgit pushed a commit that referenced this pull request May 9, 2017
dlmarion added a commit that referenced this pull request Oct 7, 2022
Added missing permission checks to ClientServiceHandler Thrift API methods
that retrieve the system/namespace/table properties, versioned properties, or
configuration. The system user will be able to call these methods. To get the
system properties / configuration, the caller will need SystemPermission.SYSTEM.
To get the namespace properties / configuration, the caller will need
NamespacePermission.ALTER_NAMESPACE on the namespace. To get the
table properties / configuration, the caller will need TablePermission.ALTER_TABLE
on the table. One side effect of this change is that when cloning a table, if the user
is not the table creator, then the user will need either TablePermission.ALTER_TABLE
on the source table or NamespacePermission.ALTER_NAMESPACE on the namespace.
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.

3 participants