Tim Armstrong has posted comments on this change.

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

Patch Set 6:


File be/src/common/init.cc:

Line 29: #include "util/openssl-util.h"
> move before "util/os-info.h"
Ran clang-format to fix the ordering.

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 
Turns out there's an OpenSSL function that tells us whether its happy. Added a 
new test that checks this while generating keys.

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

File be/src/util/openssl-util.cc:

Line 31: using strings::Substitute;
> taken care of by common/names.h?
It's not in there, so I added it and removed all the cases where it wasn't 
needed elsewhere.

Line 78:     SeedOpenSSLRNG();
> Is the RNG a thread-local or global thing? I'm wondering if we can make key
The RNG is global. Was the concern performance?

Line 130:   // Finalize encryption.
> Need to update comment since it may be finalising decryption too.

File be/src/util/openssl-util.h:

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

Line 33: /// The hash of a data buffer used for checking integrity.
Augmented this comment with details of hash algo.

Line 35:  public:
> Add an initialized_ or similar to make sure we don't Verify() against an un
I thought about it and it seemed mostly not useful, since verification against 
the garbage hash will fail.

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

Line 48: /// data. This should be regenerated for each buffer of data.
Augmented this comment with details of encryption algo.

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