Basic external file encryption properties#10
Conversation
…ructor, not doing much with it yet
sofia-tekdatum
force-pushed
the
basic_external_file_enc_properties
branch
from
a72e0ba to
8c51662
Compare
sofia-tekdatum
requested review from
argmarco-tkd,
avalerio-tkd and
cristekdatum
sofia-tekdatum
changed the title
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Overall LGTM. Left a bunch of minor comments.
A high level comment: tests are missing. Are those suppossed to be part of this review, or are they coming at a later time?
| /// remote service call. | ||
| /// For security, these values should never be sent in this config, only the locations of | ||
| /// the files that the external service will know how to access. | ||
| Builder* connection_config(const std::map<std::string, std::string>& config); |
There was a problem hiding this comment.
nit: for a while (since the design discussion), I have been debating the name of this field. 'connection_config' implies going over a network (not always the case). One just came to mind: 'instantiation_config' may be a better name. Not a blocker for this PR, this is "easily" changeable in a later iteration.
There was a problem hiding this comment.
Ack. I prefer connection_config, but we can discuss later.
| /// For security, these values should never be sent in this config, only the locations of | ||
| /// the files that the external service will know how to access. |
There was a problem hiding this comment.
nit: not sure I'd keep these two lines of comments. ("for security" may spark all sorts of debates on whether this is a truly secure way of doing things, etc")
There was a problem hiding this comment.
Simplified the comments.
| return this; | ||
| } | ||
|
|
||
| if (app_context_.size() != 0) { |
There was a problem hiding this comment.
style preference: exception-throwing stuff should probably go earlier in the method (same in other places). The way I read it "don't even try to set this value (in spite of what the value is), as it's already been set". Not a blocker from my side.
There was a problem hiding this comment.
sg, thanks!
Yes, the tests are coming with the next change where I actually start creating objects of this type. |
github-actions
Bot
added
Component: C++
Component: Parquet
awaiting review
awaiting committer review
and removed
awaiting review
labels
avalerio-tkd
left a comment
avalerio-tkd
left a comment
There was a problem hiding this comment.
Discussed offline. LGTM. Thanks!
3 participants
Adding the ExternalFileEncryptionProperties.
Subclass of FileEncryptionProperties, now just adding the class definition, builder, constructor.