Adding latest DBPAInterface to dev-miniapp#65
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
argmarco-tkd
force-pushed
the
dev_dll_work
branch
from
4b6a946 to
539db21
Compare
argmarco-tkd
force-pushed
the
dev_dll_work
branch
from
539db21 to
81c2682
Compare
argmarco-tkd
requested review from
avalerio-tkd,
cristekdatum and
sofia-tekdatum
avalerio-tkd
left a comment
avalerio-tkd
left a comment
There was a problem hiding this comment.
Thanks for the change. Nothing blocking, just a few questions and comments.
|
|
||
| class EncryptionResult { | ||
| }; | ||
| using tcb::span; |
| std::unique_ptr<EncryptionResult> DBPATestAgent::Encrypt( | ||
| span<const uint8_t> plaintext, | ||
| span<uint8_t> ciphertext) { | ||
| span<const uint8_t> plaintext) { |
There was a problem hiding this comment.
At what point will the tcb::span be converted into arrow::span? This is still tcb::span at this point, right?
There was a problem hiding this comment.
Correct. At this point, this is still tcb::span. There is no "conversion" per-se between span types - there will be copying happening at the ExternalEncryptorImpl and ExternalDecryptorImpl level. (that will be coming in another PR a bit later)
| create_encryptor_t create_instance_fn = reinterpret_cast<create_encryptor_t>(symbol_result.ValueOrDie()); | ||
|
|
||
| //at this point, we have the create_instance function pointer (from the shared library) | ||
| // at this point, we have the create_instance function pointer (from the shared library) |
There was a problem hiding this comment.
(Not specific to this line but using to ask)
Is there anything in particular we do so that we have class variables for the DBPA implementations? This is so that we can have the shared connections, auths, etc in the future? Not inmediately related to the PR, and this for my understanding. We can chat offline as well.
There was a problem hiding this comment.
Happy to chat, but in summary - we don't need to do anything special to have class variables for DBPA implementations. As long as the classes are loaded from the same shared library (i.e. the same*.so file) - things should work as if the code was compiled/linked directly with the app. I added a note in this doc
| @@ -0,0 +1,618 @@ | |||
|
|
|||
There was a problem hiding this comment.
About the file location of external resources, we may want to put them in a directory exclusively for truly external files, not mixed with other things. Not super sure if that's the case. I see cpp/src/parquet/encryption/external/loadable_encryptor_utils_test.cc for example in external and not sure if that is truly that.
There was a problem hiding this comment.
Currently, within the Arrow codebase, we have three files that "should not be there": span.hpp, dbpa_utils.h and enums.h (the latter two being copies from the DataBatchProtectionService repo). I thought thought of doing that (having a special directory for "header files that should not be here"), but given the number of files I found it to be slightly overkill. Given the push, I will make the change - but if not a blocker, I'll send a separate PR.
Now, the external directory here exists for a different reason - to contain "external encryptor/decryptor-related stuff" (e.g. dbpa loaders, etc).
And yes - loadable_encryptor_utils_test.cc is there intentionally - the actual file (loadable_encryptor_utils.cc) belongs to the external dir, and the C++ common practice is to keep the test files right next to the files they test :).
github-actions
Bot
added
awaiting review
awaiting committer review
and removed
awaiting review
awaiting committer review
labels
avalerio-tkd
left a comment
avalerio-tkd
left a comment
There was a problem hiding this comment.
LGTM in general. If concerns are addressed feel free to merge.
2 participants
Rationale for this change
In this change, we are merging-in the latest version of DataBatchProtectionAgentInterface into the mini-app code branch
What changes are included in this PR?
dbpa_interface.h(and some of the required dependencies, likeenums.h)tcb::span(instead ofstd::span, which cannot be used given that Arrow is C++17 andstd::spanis C++20)dbpa_library_wrapperto also decorate theinit()method (this method was not considered in the earlier version ofdbpa_interface). Modified the tests to includeinit()EncryptionResultandDecryptionResultare now well defined.dbpa_library_wrapper, moved some of the decorated methods to the*.hfile and inlined them.Are these changes tested?