Alex Behm has posted comments on this change.

Change subject: IMPALA-4118: extract encryption utils from BufferedBlockMgr
......................................................................


Patch Set 6:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/4389/6/be/src/common/init.cc
File be/src/common/init.cc:

Line 29: #include "util/openssl-util.h"
move before "util/os-info.h"


http://gerrit.cloudera.org:8080/#/c/4389/6/be/src/runtime/buffered-block-mgr-test.cc
File be/src/runtime/buffered-block-mgr-test.cc:

Line 387:     const int num_iterations = 10000;
thanks


http://gerrit.cloudera.org:8080/#/c/4389/6/be/src/util/openssl-util-test.cc
File be/src/util/openssl-util-test.cc:

Line 79: TEST_F(OpenSSLUtilTest, EncryptInPlace) {
Is there an easy way to inspect the RNG? Would be nice to validate that we 
actually re-seed it properly.


Line 154:   impala::InitCommonRuntime(argc, argv, true, 
impala::TestInfo::BE_TEST);
I think you can pass "false" here since you don't need an embedded JVM

You can also use the IMPALA_TEST_MAIN(); macro in testutil/gtest-util.h to 
stamp out the main()


http://gerrit.cloudera.org:8080/#/c/4389/6/be/src/util/openssl-util.cc
File be/src/util/openssl-util.cc:

Line 31: using strings::Substitute;
taken care of by common/names.h?


Line 78:     SeedOpenSSLRNG();
Is the RNG a thread-local or global thing? I'm wondering if we can make 
keys_generated non-atomic.


http://gerrit.cloudera.org:8080/#/c/4389/6/be/src/util/openssl-util.h
File be/src/util/openssl-util.h:

Line 23: #include <sstream>
move to .cc


Line 35:  public:
Add an initialized_ or similar to make sure we don't Verify() against an 
uninitialized hash_


Line 41:   bool Verify(const uint8_t* data, int64_t len);
const method


Line 68:   Status Encrypt(const uint8_t* data, int64_t len, uint8_t* out);
const function?


Line 74:   Status Decrypt(const uint8_t* data, int64_t len, uint8_t* out);
const function?


-- 
To view, visit http://gerrit.cloudera.org:8080/4389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to