search

apache / accumulo-proxy

Apache Accumulo Proxy
https://accumulo.apache.org
Apache License 2.0
9 stars 19 forks source link

Update to accumulo version 2.1 #42

Closed DomGarguilo closed 1 year ago

DomGarguilo commented 1 year ago

Closes #33

This PR

I tried to break these changes into several commits to make reviewing easier. The commit names should make it evident but there is probably no need to look at bd109d8a25f6f680a33b0e9de37d81d59552e361 since it is all automated thrift changes. The main commit to review would be 69d5120fdc3943141c6edef8b4dd3e7ec9d21d92

DomGarguilo commented 1 year ago

There are a few side affects from the changes in this PR. Some good, some bad. The two ITs that were failing previously now pass (#37 & #39). But now two other ITs are failing testSiteConfiguration and userAuthentication. Also I think logging is broken with this change due to log4j/slf4j conflicts. I can try to fix and incorporate the things that are broken into this PR or, since its already large, and to limit the scope of this PR, merge this as is and create follow on tickets for them.

ctubbsii commented 1 year ago

But now two other ITs are failing testSiteConfiguration and userAuthentication.

I'm not 100% sure what these two do based on their name, but one of the intents when splitting this off into its own repo was try to try to make it solely an AccumuloClient for a single user.

So, it should not need to have access to Accumulo's site configuration to work. Any attempt to try to make that work, and related tests should be removed. It sounds like testSiteConfiguration might be related to that.

To operate as a single user, an instance should only need access to a client properties file, and should authenticate to Accumulo using credentials stored there. We no longer need to provide separate re-authentication mechanisms, like a "login" command in the proxy, or anything like that. It should just need to do Accumulo.newClient().from(properties).build(). So, if the userAuthentication test tries to do anything else, it might need to be updated or removed.

So, if those tests are related to those behavior changes from earlier versions, I would not be surprised if they need to be updated, or even removed.

Also I think logging is broken with this change due to log4j/slf4j conflicts.

I think that should be relatively easy to fix, but let me know; I'm willing to try to help if it gets tricky.

I can try to fix and incorporate the things that are broken into this PR or, since its already large, and to limit the scope of this PR, merge this as is and create follow on tickets for them.

I would create subsequent patches, rather than try to do everything in one.

DomGarguilo commented 1 year ago

Also I think logging is broken with this change due to log4j/slf4j conflicts.

I think that should be relatively easy to fix, but let me know; I'm willing to try to help if it gets tricky.

@ctubbsii, I created #45 to track this issue. I have looked into it a bit and am not too sure how to fix it right now. If you would like to take a look, feel free.