I skimmed through and this seems like a clean design (I would have to reread the PR to do a comparison. A few thoughts of the top of my head:
> - Multiple internal classes are left public in header files, where it > would be > preferred that public classes be kept to a minimum. I think some of these classes are non-trivial. If that is the case it would be preferable to put them in an internal namespace to ensure adequate unit testing is in place. It seems like KmsClient and maybe PropertiesDrivenCryptoFactory should still be implemented with internal synchronization to guarantee thread-safety? On Wed, Nov 11, 2020 at 6:19 PM Benjamin Kietzman <bengil...@gmail.com> wrote: > In the context of https://issues.apache.org/jira/browse/ARROW-9318 / > https://github.com/apache/arrow/pull/8023 which port the parquet-mr > design to c++: there's some question whether that design is consistent > with the style and conventions of the c++ implementation of parquet. > > Here is a Gist with a sketched alternative design adapted to c++ > https://gist.github.com/bkietz/f701d32add6f5a2aeed89ce36a443d43 > > Specific concerns in brief: > - Multiple internal classes are left public in header files, where it > would be > preferred that public classes be kept to a minimum. > - Several levels of explicit synchronization with mutexes are used to > guarantee > that each class is completely thread safe, but this is unnecessary > overhead > given the pattern of use of `parquet-cpp`. >
