search

apache / accumulo-proxy

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

Fix #3 Updated the accumulo-proxy with latest from accumulo (pre-2.0) #4

Closed hkeebler closed 5 years ago

hkeebler commented 5 years ago

This builds and verifies with the updates from the accumulo project. The IT tests were moved over and execute. Not sure if more testing needs to be done.
The couple of quirky things are: Added the Encoding exculsion to ProxyDurabilityIT test. This was not necessary in the accumulo project and I'm not sure how it was excluded there.

@SuppressFBWarnings(value = {"HARD_CODE_PASSWORD", "DM_DEFAULT_ENCODING"},
      justification = "test password is okay and no check needed on encoding")

Added the Throwable in ProxyServer because it did not pass some code check.

catch (Throwable e) {
      handleException(new Exception(e));
ivakegg commented 5 years ago

Added the Throwable in ProxyServer because it did not pass some code check.

catch (Throwable e) {
      handleException(new Exception(e));

Catching Throwable is a very dangerous thing to do. We need to fix the verify to allow catching exception here instead.

ivakegg commented 5 years ago

Apparently the generate-thrift script was not part of this pull request. However the following difference was interesting:

diff accumulo/core/src/main/scripts/generate-thrift.sh accumulo-proxy/src/main/scripts/generate-thrift.sh I saw the following: thrift ${THRIFT_ARGS} --gen java:generated_annotations=suppress "$f" || fail unable to generate java thrift classes was changed to: thrift ${THRIFT_ARGS} --gen java:generated_annotations=undated,handle_runtime_exceptions "$f" || fail unable to generate java thrift classes

hkeebler commented 5 years ago

@ivakegg updated the throwable with the spotbugs recommended solution below. It now also passes the verify. } catch (RuntimeException e) { throw (e); } catch (Exception e) { handleException(e); return null;

hkeebler commented 5 years ago

Clicked the wrong button again. My apologies. This is still an open pull request. The thrift build was verified and this last commit will include the generated_annotations=suppressed argument in the generate_thrift.sh.

ctubbsii commented 5 years ago

Totally lost track of this PR. Merged today. Doesn't look like all the ITs are passing, but can be fixed in follow-on.