IOSS: add an S3 database component #725
Conversation
There was a problem hiding this comment.
Looks good. Lots of work in there. I added several comments; some suggestions and some changes I would either like to see or discuss.
I may have additional comments later.
It would be good to change a lot of the IOSS_ERROR that use a ostringstream to just pass the hardwired string directly to IOSS_ERROR. Would reduce lots of code and make the error handling less obtrusive. (The change to have IOSS_ERROR accept a string probalby happened during/after you were working on this).
I would also like to see if the use of int rc could be changed to bool error...
Pushback is accepted on any/all suggestions...
There was a problem hiding this comment.
Just putting this here so I can remember. When building on a Mac, I had to add libaws-crt-cpp.dylib to TPL_AWSSDK_LIBRARIES:PATH
Ioss: TPL_ENABLE_AWSSDK -> TPL_ENABLE_AWSSDK AND TPL_ENABLE_Cereal
* make a local nonconst copy of the PropertyManager * populate it with the existing Properties from const PropertyManager * get any S3 parameters from environment variables and add to the PropertyManager * populate the S3 parameter struct from the PropertyManager
use IOSS_ERROR for unrecoverable errors remove unnecessary error message
add a PackedBytes type alias to clarify pack/unpack vector usage
use ${CMAKE_SHARED_LIBRARY_SUFFIX} instead of hardcoded .so
add "lib" to the list of install dirs to look in for AWS libraries only enable S3 if both AWSSDK and Cereal are found
remove cmake-s3 script in favor of cmake-config.
change result var name from rc to success
change map_properties result name to num_failed.
properties to params: respect boolean values not just existence of the property
tkordenbrock
force-pushed
the
tkordenbrock/add-ioss-s3
branch
from
17eae82 to
25a08dc
Compare
tkordenbrock
force-pushed
the
tkordenbrock/add-ioss-s3
branch
from
700de98 to
a32578d
Compare
|
@gsjaardema I think I addressed all your concerns. Let me know what you think. I pushed a lot of small commits so we could revisit or reverse any of the changes. I would prefer to squash them into a single commit when this PR is merged. |
tkordenbrock
force-pushed
the
tkordenbrock/add-ioss-s3
branch
from
1cd0ca1 to
cc22d2a
Compare
|
Thanks for doing this and putting up with all my (nit-picky) comments. Looks good and i wish I had more time left to work with this and use it. |
|
I appreciate the thorough review. It made for better code. Best of luck in the future! |
No description provided.