impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4118: extract encryption utils from BufferedBlockMgr
Date Thu, 22 Sep 2016 07:06:53 GMT
Tim Armstrong has posted comments on this change.

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

Patch Set 6:

File be/src/common/

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

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, impala::TestInfo::BE_TEST);
> I think you can pass "false" here since you don't need an embedded JVM
File be/src/util/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message