feat(storage): add support for conditional idempotent operations

[bajajneha27]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea.
• Follow the instructions in CONTRIBUTING. Most importantly, ensure the tests and linter pass by running bundle exec rake ci in the gem subdirectory.
• Update code documentation if necessary.

closes: #<issue_number_goes_here>

[dazuma]

consider:

def self.retry? query_params
  query_params.any? { |_key, val| !val.nil? }
end

[dazuma]

This seems like it should be a module, not a class, since there's no point in instantiating it. Also, if the module exists only to contain one helper method, maybe that method should just be a private helper method in the service class where it's being called from. It seems like it's not meant to be a part of the public interface.

[bajajneha27]

Yeah that makes sense. I kept it in a separate file because I thought it'd be used from multiple places.

[tritone]

Seems like a step in the right direction. Remind me, have you added user configuration to allow users to retry in all cases yet? This will be helpful if any users were depending on this current behavior, so it should be a fast follow from this PR if it's not already complete.

[tritone]

Note that this should be for the destination file, not the source-- not sure how this is distinguished in Ruby.

[bajajneha27]

I believe this is for destination file and if_source_generation_match is for source file.

[tritone]

I see you updated mocks for existing tests; is it possible to also add a simple test with preconditions to ensure that retries do occur in this case? I guess this will be covered by conformance tests as well.

[bajajneha27]

I did think of that. I'm not sure how I can verify if the method is being retried over here but then I thought this would be covered in conformance test. I think that would suffice, no?

[tritone]

Yup conformance tests should cover it

[tritone]

Same here, compose also requires preconditions for the destination specifically in order to allow retries.

[bajajneha27]

Same goes here. if_source_generation_match is for source files and if_generation_match is for destinations files.
@dazuma @quartzmo can you please confirm if you have any idea?

[dazuma]

I believe that's correct.

[bajajneha27]

Seems like a step in the right direction. Remind me, have you added user configuration to allow users to retry in all cases yet? This will be helpful if any users were depending on this current behavior, so it should be a fast follow from this PR if it's not already complete.

We by default used to retry all the operations. Now, we're trying not to retry the conditionally idempotent operations.
Not sure if I got your question and answered right.

[bajajneha27]

The acceptance and samples tests failures are happening on the main branch too and are unrelated to my changes. I've created the issue#18854 to track this.

[tritone]

Seems like a step in the right direction. Remind me, have you added user configuration to allow users to retry in all cases yet? This will be helpful if any users were depending on this current behavior, so it should be a fast follow from this PR if it's not already complete.

We by default used to retry all the operations. Now, we're trying not to retry the conditionally idempotent operations. Not sure if I got your question and answered right.

Changing the default to only retry idempotent ops is correct. My questions is, can users override this default? Having this configurability is important so users can choose to retry everything if they don't like the default behavior.

[bajajneha27]

Seems like a step in the right direction. Remind me, have you added user configuration to allow users to retry in all cases yet? This will be helpful if any users were depending on this current behavior, so it should be a fast follow from this PR if it's not already complete.

We by default used to retry all the operations. Now, we're trying not to retry the conditionally idempotent operations. Not sure if I got your question and answered right.

Changing the default to only retry idempotent ops is correct. My questions is, can users override this default? Having this configurability is important so users can choose to retry everything if they don't like the default behavior.

No, not with this change. I thought the requirement was not to let users retry the conditionally idempotent & non-idempotent operations even if they want to.
But if that's not the case, I'll have to make more changes.

[tritone]

Overall LGTM, please add some docs for the retry option.

[tritone]

Could you add docs for the option? Also just wanted to confirm that this will be something you can add across the board to library methods.

[bajajneha27]

I updated OVERVIEW doc which explains how to use the options field overall. Also updated other methods to accept options. We've a separate PR raised already for non-idempotent operations.

[tritone]

Generally LGTM

[tritone]

Also include information about how users can enable retries on non-idempotent operations. (this is possible right?)

[bajajneha27]

We'll be adding it in a separate PR which covers retry configurations for non-idempotent operations overall. I didn't want to club that here because Sandeep has already worked on it.