Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s3, allow to ignore get/put errors #54

Merged
merged 2 commits into from
May 27, 2024
Merged

Conversation

russki
Copy link

@russki russki commented May 23, 2024

@breuner 🙏

partially resolves #52

Copy link
Owner

@breuner breuner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i noticed you haven't added this option for multipart uploads. but that's ok. if you fix the minor issues that i mentioned in the comments, then i can merge this and will work on top of your patch to add this to other IO operations and to add the log file option, as discussed in #52. (or you can of course also try adding the log file option if you prefer.)

@@ -146,6 +146,7 @@ namespace bpt = boost::property_tree;
#define ARG_S3BUCKETACLPUT_LONG "s3baclput"
#define ARG_S3ENDPOINTS_LONG "s3endpoints"
#define ARG_S3FASTGET_LONG "s3fastget"
#define ARG_S3IGNOREERRORS_LONG "s3ignoreerrors"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alignment of the word "s3ignoreerrors" not correct.
tab width is 4. i actually switched from tabs to 4 spaces recently for new patches. but not a showstopper if you keep tabs for this patch (because i know i should really write a code style guide for these things. some day... :-) )

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, sorry. some of the parts of the file was still tabs and some other spaces. would love the mega patch from you to standardize at some point <3

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm still thinking about that mega patch, but probably won't do it, because this change to all code lines would make "git blame" harder and thus i wouldn't even know who to yell at if there ever is a bug in the code ;-)

"Exception: " + s3Error.GetExceptionName() + "; " +
"Message: " + s3Error.GetMessage() + "; " +
"HTTP Error Code: " + std::to_string( (int)s3Error.GetResponseCode() ) );
if (!ignoreS3Errors) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opening curly bracket ({) to new line

"Exception: " + s3Error.GetExceptionName() + "; " +
"Message: " + s3Error.GetMessage() + "; " +
"HTTP Error Code: " + std::to_string( (int)s3Error.GetResponseCode() ) );
if (!ignoreS3Errors) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opening curly bracket ({) to new line

@@ -496,6 +496,8 @@ void ProgArgs::defineAllowedArgs()
"Send downloaded objects directly to /dev/null instead of a memory buffer. This option "
"is incompatible with any buffer post-processing options like data verification or "
"GPU data transfer.")
/*s3f*/ (ARG_S3IGNOREERRORS_LONG, bpo::bool_switch(&this->ignoreS3Errors),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/*s3f*/ needs to be changed to /s3i*/. as you were probably aware already, these comments are just for us humans to make it easier to find the right place so that the --help-all output is sorted alphabetically by option strings.

@russki
Copy link
Author

russki commented May 25, 2024

@breuner added ignores for multipart uploads as well, plz let me know if there were any other s3 put/get operations i missed

yeah, maybe we can merge this first and then do the logging part

@breuner breuner merged commit 478c19c into breuner:master May 27, 2024
10 checks passed
@breuner
Copy link
Owner

breuner commented May 27, 2024

thanks for your contribution! if you want to tell me your real name for this then i'll add that to the thanks in the changelog. otherwise i'll just use your github username there.

@russki
Copy link
Author

russki commented May 28, 2024

thanx @breuner! github username is just fine

any chance this can be pulled into 3.0.8 and released soon? 🙏

@breuner
Copy link
Owner

breuner commented May 29, 2024

indeed i'm planning to build a new release in the couple of days. (it's still a bit of manual work for me to make a release, because i enjoy writing C++ code more than writing automation scripts, and thus i'm making releases a bit less frequently than i should, but i'm working on improving this :-) ).

since you mentioned 3.0.8, i should tell you a secret (although it shouldn't be a secret, i just didn't find a good way to document it): i'm using even version numbers for "work in progress" while i'm still gathering features in between normal releases. and the normal releases have an odd last digit. that's why you'll only find v3.0.1, v3.0.7 and such in the releases section here in github, but not v3.0.0 or v3.0.4 and such.
in this spirit, the next release will be v3.0.9.

@breuner
Copy link
Owner

breuner commented Jun 4, 2024

hi @russki, i just wanted to let you know that this is now included in the latest elbencho v3.0.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Continue on when encountering s3 errors
2 participants