cloud_storage_clients: Fix xml parsing when searching for error code in response #14848
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iobuf_to_delete_objects_result is moved to public API to make testing it easier.
When searching for error codes in an XML tree, ptree::count does not honor dot paths. The change uses get_optional and adds some tests for the method.
abhijat
force-pushed
the
issue/14014/fix-xml-parsing
branch
from
a94bc31
to
1ec6cfe
Compare
VladLazar
approved these changes
Nov 9, 2023
|
ducktape was retried in job https://buildkite.com/redpanda/redpanda/builds/40759#018bb31b-13c7-491d-8007-ad02ab6bce64 |
|
/backport v23.2.x |
nvartolomei
approved these changes
Nov 9, 2023
| @@ -243,4 +243,7 @@ class s3_client : public client { | |||
| ss::shared_ptr<client_probe> _probe; | |||
| }; | |||
|
|
|||
| std::variant<client::delete_objects_result, rest_error_response> | |||
| iobuf_to_delete_objects_result(iobuf&& buf); | |||
There was a problem hiding this comment.
i know you didn't add this function here, but you are making it "public" so i believe the following comment is appropriate
This function can have a better name.
x_to_*usually means a type conversion/format of some sort, not the case here- this method expects the input to be xml, the method doesn't say anything about the input expectations
something along the line of parse_delete_objects_result is much better name
- it tells what it is doing: parsing
- it tells the input expectations: delete objects result (from the context (ie the namespace location) we know that it must be a cloud storage delete objects result
-
- i'll correct myself
parse_**s3**_delete_objects_resultis a better name, it won't work for azure but since it is in the same namespace it might confuse users
- i'll correct myself
- iobuf is a detail and is part of the type declaration, we don't need it in the name. moreover, we can have multiple overloads for different type of "input carriers" and won't need to have dozen of function names
| iobuf_to_delete_objects_result(iobuf&& buf) { | ||
| auto root = util::iobuf_to_ptree(std::move(buf), s3_log); | ||
| auto result = client::delete_objects_result{}; | ||
| try { | ||
| if (root.count("Error.Code") > 0) { | ||
| if (auto error_code = root.get_optional<ss::sstring>("Error.Code"); |
There was a problem hiding this comment.
I don't expect you to change anything here, but (I believe) we can save some CPU instructions by doing extracting the child object here
if( auto error_payload = root.get_child_optional("Error"); error_payload.has_value() {
...
auto code = error_payload.get<ss::sstring>("Code");
}changes:
- remove tree walking for each property
- remove the empty string optional and let it throw to show the expectations and intentions
-
- these properties must always be present, if they are not we are likely doing something wrong or parsing a bogus message
This was referenced Nov 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FIXES: #14014
When searching for error codes in an XML tree, ptree::count which we used does not honor dot paths. The change uses get_optional and adds some tests for the method.
Backports Required
Release Notes