rgw/sfs: refactor of SFSBucket::merge_and_store_attrs()

[giubacc]

Refactored SFSBucket::merge_and_store_attrs() with "overwrite" semantics.
The previous implementation was started as if the method should implement a merge but the actual usage suggests "overwrite" semantics.
The new implementation is more concise and simplified.

Fixes: https://github.com/aquarist-labs/s3gw/issues/798
Signed-off-by: Giuseppe Baccini [email protected]

Contribution Guidelines

• To sign and title your commits, please refer to Submitting Patches to Ceph.

Checklist

• Tracker (select at least one)
  • References tracker ticket
  • Very recent bug; references commit where it was introduced
  • New feature (ticket optional)
  • Doc update (no ticket needed)
  • Code cleanup (no ticket needed)
• Documentation (select at least one)
  • Updates relevant documentation
  • No doc update is appropriate
• Tests (select at least one)
  • Includes unit test(s)
  • Includes integration test(s)
  • Includes bug reproducer
  • No tests

[jecluis]

I think it just needs code documentation. Otherwise lgtm.

[giubacc]

I think it just needs code documentation. Otherwise lgtm.

ok, I will add a comment explaining a bit our current state of understanding regarding this.

[irq0]

set semantics: This confused me on first read. You mean set as in setter not as in set theory, right? Suggest to use 'overwrite' in the docstring / commit message

[irq0]

The base class doc string says

 /** Set the attributes in attrs, leaving any other existing attrs set, and
 * write them to the backing store; a merge operation */

My read of the usage in rgw_op.h would be the same. Usually the code adds one or more attributes to the existing set of attributes.

Why is overwriting the attr map the correct behavior?

[giubacc]

The base class doc string says

 /** Set the attributes in attrs, leaving any other existing attrs set, and
 * write them to the backing store; a merge operation */

My read of the usage in rgw_op.h would be the same. Usually the code adds one or more attributes to the existing set of attributes.

Why is overwriting the attr map the correct behavior?

This is my final thought after reaching the end of this: https://github.com/aquarist-labs/s3gw/issues/750
All started because @0xavi0 added a piece of code that was contradicting my understanding of a merge .
But in the end, that was the correct patch because callers are using the method as a setter (so yes, set not as a mathematical set, but meaning a setter() :) ).

Honestly, reading this specification:

/** Set the attributes in attrs, leaving any other existing attrs set, and
     * write them to the backing store; a merge operation */

I have doubts that implementing this as a setter() is the right thing to do, but again, the actual usages are using this as a set operation, so we have to implement this way if we want s3-tests to pass.

[jecluis]

I have doubts that implementing this as a setter() is the right thing to do, but again, the actual usages are using this as a set operation, so we have to implement this way if we want s3-tests to pass.

To be honest, I'm more concerned about getting the right behavior than doing things just to have s3-tests passing.

It seems to me that what was documented in the SAL and what is expected are two different things. One of them is a bug. I'm inclined to believing that the documentation is wrong, as well as the function name, because the s3-tests seem to expect a different behavior than what is described.

I think for us to merge this we will need a better explanation in our implementation's documentation. We need to provide context to justify what we are doing, other than just saying that the actual usage is intended as an "overwrite" operation -- intended by whom?

Think about it this way: the whole team leaves the project, and whomever comes along will need to understand why this is being used as a setter. The documentation on the SAL is not agreeing with our assertion. There must be more context given to that person/AI so they understand what the problem is.

Once we provide more context on this, I think we can merge the PR.

[giubacc]

I have doubts that implementing this as a setter() is the right thing to do, but again, the actual usages are using this as a set operation, so we have to implement this way if we want s3-tests to pass.

To be honest, I'm more concerned about getting the right behavior than doing things just to have s3-tests passing.

Me too, the whole issue has started because a piece of code was added a certain point and has transformed this merge_and_store_attrs method into a set_and_store_attrs.
So I've decided to go deep into understanding the roots of the problem.

It seems to me that what was documented in the SAL and what is expected are two different things. One of them is a bug. I'm inclined to believing that the documentation is wrong, as well as the function name, because the s3-tests seem to expect a different behavior than what is described.

Yup, I'm inclined to think this is just a SAL method-name and SAL documentation issue.

I think for us to merge this we will need a better explanation in our implementation's documentation. We need to provide context to justify what we are doing, other than just saying that the actual usage is intended as an "overwrite" operation -- intended by whom?

Think about it this way: the whole team leaves the project, and whomever comes along will need to understand why this is being used as a setter. The documentation on the SAL is not agreeing with our assertion. There must be more context given to that person/AI so they understand what the problem is.

Once we provide more context on this, I think we can merge the PR.

You are right, these are exactly my same considerations, but keep in mind that the current code we have is far worst that the patch / comment we would like to introduce with this patch.
With this patch we are expressing clearly in our implementation an 'overwrite' behavior and introducing a doubt about the SAL naming / docs.
Could be reasonable to merge this and contextually open an issue upstream asking for clarifications?

[jecluis]

You are right, these are exactly my same considerations, but keep in mind that the current code we have is far worst that the patch / comment we would like to introduce with this patch. With this patch we are expressing clearly in our implementation an 'overwrite' behavior and introducing a doubt about the SAL naming / docs.

Right, but it can be better. We can actually convey our current understanding. I'm just asking for a more detailed explanation on our current rationale.

Could be reasonable to merge this and contextually open an issue upstream asking for clarifications?

I believe asking for said clarification is reasonable, but I also think adding more context before merging this and leave it consigned to oblivion would be reasonable as well. The two are not mutually exclusive.