Disable Return of Affected Records Count for Update and Delete Operations in NoSQL Databases

[otaviojava]

Unlike relational databases, which consistently return the number of affected records, many NoSQL databases, especially those with eventual consistency, do not guarantee this behavior. This PR addresses the issue by disabling the return of affected record counts for NoSQL operations, opting for zero or a consistent placeholder value instead. This ensures compatibility and prevents problems due to unreliable count data in systems like Couchbase, Elasticsearch, Cassandra, and Redis.

By adopting this change, the application will handle updates and deletes uniformly across different NoSQL databases, avoiding problems associated with inconsistent or unavailable record counts.

[njr-11]

I noticed a few lines up that the test does invoke the following wait which Kyle added to allow for eventual consistency:

        TestPropertyUtility.waitForEventualConsistency();

Are you running the TCK with the option to set a wait time?
And if so, is the mechanism insufficient in some way, or is the database not even capable of counting a number of deletions?

If it isn't capable and will never give the right answer, the repository operation should be made to raise UnsupportedOperationException to alert the user of this, and the test case can trap for it, allowing that exception for those types of databases.

[otaviojava]

it does make sense.

[otaviojava]

About what I said, I was wrong.

Even with TestPropertyUtility.waitForEventualConsistency(), it won't make sense because this behavior will not be guaranteed in real life or production.

Because the result will vary depending on the amount of data, replica strategy, and number of database nodes.

In this case, I recommend you enhance the documentation to explain that this return will vary according to NoSQL databases, mainly for eventual consistency.

[njr-11]

Yes, documentation needs to be updated regardless. We have #782 for that. I added a checkbox to it for this.

[otaviojava]

@njr-11 could you review it now?

[njr-11]

I added suggestions for fixing a couple of problems in the proposed approach. First, the UnsupportedOperationException should only be permitted when using eventual consistency. We can find that out by checking for the presence of the test property for the eventual consistency delay. Second, once the UnsupportedOperationException is ignored, we need to address the fact that the remove didn't happen because the remainder of the test assumes that it worked.

[otaviojava]

I added suggestions for fixing a couple of problems in the proposed approach. First, the UnsupportedOperationException should only be permitted when using eventual consistency. We can find that out by checking for the presence of the test property for the eventual consistency delay. Second, once the UnsupportedOperationException is ignored, we need to address the fact that the remove didn't happen because the remainder of the test assumes that it worked.

Ok, I will test.

But IMHO, it should not be thrown an UnsupportedOperationException, but the database's information at that time should be even zero.
Eventual persistence does not mean that the data won't be removed, but it will not happen like in an ACID database.

[njr-11]

Eventual persistence does not mean that the data won't be removed, but it will not happen like in an ACID database.

It's not eventual consistency that means the data won't be removed. It's that if you invoke a method and it raises an UnsupportedOperationException back to you, you should have the expectation that the method probably didn't do anything.

[otaviojava]

Eventual persistence does not mean that the data won't be removed, but it will not happen like in an ACID database.

It's not eventual consistency that means the data won't be removed. It's that if you invoke a method and it raises an UnsupportedOperationException back to you, you should have the expectation that the method probably didn't do anything.

In that case, I would change the solution this one.
So, if there is not eventual set it should skip this one.
What do you think?

[njr-11]

It's not eventual consistency that means the data won't be removed. It's that if you invoke a method and it raises an UnsupportedOperationException back to you, you should have the expectation that the method probably didn't do anything.

In that case, I would change the solution this one. So, if there is not eventual set it should skip this one. What do you think?

I looked at your latest commits to see what you mean by this. It looks like you mean run the long removeAll() operation regardless of whether eventual consistency, expecting that an eventual consistency database will not raise UnsupportedOperationException any more and that it will ignore the fact that it is incapable of returning a correct result. Then you will have the test avoid making an assertion on the result so that it can pass.

That seems wrong. If a database isn't capable of a thing it needs to raise UnsupportedOperationException, not return a bogus result that the user doesn't know what to do with.

You should revert 4a200ae with this change and put it back how we had it before that.

[otaviojava]

That seems wrong. If a database isn't capable of a thing

🤔 But that is my point. It is capable, but it will return as eventual consistency, not as an ACID database.

[njr-11]

That seems wrong. If a database isn't capable of a thing

🤔 But that is my point. It is capable, but it will return as eventual consistency, not as an ACID database.

No - it isn't capable. It cannot return an accurate deletion count to the user as the method signature requires. A method with signature of long removeAll() needs to raise UnsupportedOperationException when it cannot return the right number.

[otaviojava]

That seems wrong. If a database isn't capable of a thing

🤔 But that is my point. It is capable, but it will return as eventual consistency, not as an ACID database.

No - it isn't capable. It cannot return an accurate deletion count to the user as the method signature requires. A method with signature of long removeAll() needs to raise UnsupportedOperationException when it cannot return the right number.

No - it isn't capable. It cannot return an accurate deletion count to the user as the method signature requires.

Let me try another approach:

NoSQL databases are primarily designed based on the BASE principle instead of ACID in relational databases. This design ensures that read and write operations are consistently available on all nodes. Still, it sacrifices consistency, meaning that reads only sometimes return the most up-to-date data. This trade-off may prompt you to consider the implications of NoSQL databases.

There is no way to have it all in distributed databases. Any attempt to do so would violate the CAP theory, which states that consistency, availability, and partition tolerance cannot be simultaneously achieved. This understanding of trade-offs is fundamental in the discussion of distributed databases.

When you said:

No - it isn't capable. It cannot return an accurate deletion count to the user as the method signature requires. 

You are right, but when I look at BASE databases, they differ from what I seek as a NoSQL developer.

Based on that, we need to decide on the API with documentation.

Is this resource ACID or Relational database only?

If yes, we need to document that and explain that it is SQL or ACID-only behavior. As NoSQL, I will not implement or skip this test.

Is this resource also available with NoSQL (BASE)?

We still need to document this behavior, mainly because running in a single instance differs from running with many cases; that is what we usually do on NoSQL databases.

Thus, we need to put something like:

In the eventual database, this value might vary based on the number of instances or data centers that this database is running.

Mainly because if the database cannot guarantee this return, the implementation also cannot.

Based on that, starting writing NoSQL databases TCK tests, we could copy/paste most of a relational database.

I am more in favor of having specializations, mainly because, by default, as the majority of you have a relational database background, try to apply relational database as the default or expected behavior. Plus, those tests are totally based on JPA

[njr-11]

Thus, we need to put something like:

In the eventual database, this value might vary based on the number of instances or data centers that this database is running.

We don't need to document it that way. We can instead document it to say that the numeric and boolean return type cannot be used with eventual consistency databases and will result in UnsupportedOperationException. Instead use the void return type so that the user does not end up with an unreliable result that they might not realize is unreliable. That will help stop users from writing bad application code.

Unless you have some valuable use case for why a user should want an unreliable deletion count, I can't see why we should document this the way you are saying we need to document it.

[otaviojava]

Thus, we need to put something like:
In the eventual database, this value might vary based on the number of instances or data centers that this database is running.

We don't need to document it that way. We can instead document it to say that the numeric and boolean return type cannot be used with eventual consistency databases and will result in UnsupportedOperationException. Instead use the void return type so that the user does not end up with an unreliable result that they might not realize is unreliable. That will help stop users from writing bad application code.

Unless you have some valuable use case for why a user should want an unreliable deletion count, I can't see why we should document this the way you are saying we need to document it.

I love this approach, so let's work on it.

[njr-11]

I love this approach, so let's work on it.

It is already implemented under this pull if you revert commit 4a200ae and the commit on top of it.

The only other remaining piece is adding the documentation to the specification, which is covered by #782 where I have added a checkbox for this to ensure it happens.

[njr-11]

It looks like the revert attempt didn't quite restore the PR to its original 4 commits and introduced a few mistakes, so I added suggestions to correct them.

[njr-11]

Looks good now. Thanks!

[otaviojava]

ops, we forget one point.
I will create another PR, because I already merged this one.