impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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:

(13 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"
Ran clang-format to fix the ordering.


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 
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
Done


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?
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.
Done


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
Done


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
Done


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?
Done


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


-- 
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message