Update transactions documentation for native transaction support. #1513

mikereiche · 2022-07-21T19:19:56Z

Closes #1454.

Staged documentation is available at : https://mikereiche.github.io/staged/transactions.html

You have read the Spring Data contribution guidelines.
There is a ticket in the bug tracker for the project in our JIRA.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

README.adoc

src/main/java/org/springframework/data/couchbase/config/AbstractCouchbaseConfiguration.java

src/main/java/org/springframework/data/couchbase/config/BeanNames.java

src/test/java/org/springframework/data/couchbase/domain/CapellaConnectSample.java

src/main/asciidoc/transactions.adoc

daschl · 2022-07-22T08:59:19Z

src/main/asciidoc/transactions.adoc

+#rollbackForClassName/noRollbackForClassName, which allow
+rules to be specified as types or patterns, respectively.
+
+When a rollback rule is defined with an exception type, that type will be


do all these rollback rules here apply to us too? (pinging @programmatix)

There is some egregious cut-and-pasting from spring doc/java doc. None of the rollback applies.

src/main/asciidoc/transactions.adoc

daschl · 2022-07-22T09:01:37Z

src/main/asciidoc/transactions.adoc

+@Component
+class PersonService {
+
+    final ReactiveCouchbaseOperations personOperationsRx;


btw one (maybe subjective) recommendation would be to use Reactive instead of Rx, it could be confused with RxJava and is certainly lingo that some users might not understand

That's a really good point.

daschl · 2022-07-22T09:01:51Z

src/main/asciidoc/transactions.adoc

-Airline read = mappingCouchbaseConverter.read(Airline.class, source);
+    TransactionResult result = template.getCouchbaseClientFactory().getCluster().transactions().
+            run(ctx -> 
+                { 


formatting looks a bit off here?

Yes, I tried to call out the block {...} of operations in the transaction, but it just ends up being ugly. I'll let the code formatter handle it.

programmatix · 2022-07-22T08:35:43Z

src/main/asciidoc/transactions.adoc

 = Transaction Support

-Couchbase supports https://docs.couchbase.com/server/6.5/learn/data/transactions.html[Distributed Transactions]. This section documents on how to use it with Spring Data Couchbase.
+Couchbase supports https://docs.couchbase.com/server/6.5/learn/data/transactions.html[Distributed Transactions]. This section documents how to use it with Spring Data Couchbase.


Needs updating to current

Good point. I was thinking it should be the earliest version where it works, but it should indeed be "current".

programmatix · 2022-07-22T08:36:05Z

src/main/asciidoc/transactions.adoc

 = Transaction Support

-Couchbase supports https://docs.couchbase.com/server/6.5/learn/data/transactions.html[Distributed Transactions]. This section documents on how to use it with Spring Data Couchbase.
+Couchbase supports https://docs.couchbase.com/server/6.5/learn/data/transactions.html[Distributed Transactions]. This section documents how to use it with Spring Data Couchbase.

 == Requirements

 - Couchbase Server 6.5 or above.


Needs updating

Right. Same as above - while the "6.5 or above" is correct for "Requirements", this should always point to the current documentation.

No I mean the minimum version is 6.6.1 now :)

programmatix · 2022-07-22T08:41:20Z

src/main/asciidoc/transactions.adoc

 - NTP should be configured so nodes of the Couchbase cluster are in sync with time. The time being out of sync will not cause incorrect behavior, but can impact metadata cleanup.

 == Getting Started & Configuration

-The `couchbase-transactions` artifact needs to be included into your `pom.xml` if maven is being used (or equivalent).
+Couchbase Transactions are leveraged with the @Transactional operator (using the Couchbase Transaction Manager), a Couchbase Transaction Template (created from the Transaction Manager),  


Personally I wouldn't mention the last two, at least right out the gate here. I feel the biggest problem with Spring transactions is how many ways there are of doing them. A competitor has, what, 5+? I grok them now, but coming to them as a newbie was overwhelming and off-putting. We can learn from their mistakes :)

I feel the two main ways are @transactional and (my preference) just using Spring operations directly inside our regular API. While we did implement support for some other options they don't, IMO, add any real-world value to the user. If we mention them at all I feel it should be in a 'for advanced users' style section at the end, and we should list when you'd use each over the other (which would IMO quickly show that they don't offer any advantages over the core two methods above).

I'd suggest leading with a couple of short examples showing those two main styles here, before getting into the nitty-gritty of beans and configuration.

I removed a bunch of the details.

programmatix · 2022-07-22T08:48:31Z

src/main/asciidoc/transactions.adoc

-Once it is included in your project, you need to create a single `Transactions` object. Conveniently, it can be part of
-your spring data couchbase `AbstractCouchbaseConfiguration` implementation:
+Functioning of the  @Transactional method annotation requires the configuration class to be annotated 
+with @EnableTransactionManagement. The service object with the annotated methods must be obtained as an @Bean either via @Autowire or


This seems a lot of rules - maybe a list would help.
An example annotated with little number bubbles would help illustrate these.
Is "service object" defined enough as a Spring thing? E.g. rather than "Service" (capitalised)? And it's not immediately clear that it's referring to "the service with @transaction methods". An example would clear that up.

I turned this into a list.

Yep this is much easier to read now, especially with the cross-referenced numbers - thanks.

programmatix · 2022-07-22T08:49:04Z

src/main/asciidoc/transactions.adoc

+Functioning of the  @Transactional method annotation requires the configuration class to be annotated 
+with @EnableTransactionManagement. The service object with the annotated methods must be obtained as an @Bean either via @Autowire or
+from the application context. And the call to the method must be made from a different class than service because calling an annotated
+method from the same class will not invoke the Method Interceptor that does the transaction processing.


Eep, did not know that... I've tested @transactional methods calling other @transactional methods in the same Service and that seemed to work fine?

That works because you're already in a transaction from the first method. But if your first method was not an @transactional and the second method was, then the second method would not be called in a transaction.

For instance, if I add the method below to ReactiveTransactionalTemplateIntegrationTests, and change the test to call it instead of calling doInTransaction() directly - the tests fail as they are not in a transaction - the @transactional of doInTransaction has no effect as doInTranaction() does not get the CglibAOP treatment.

public Mono<Void> doNotInTransaction(AtomicInteger tryCount, Function<ReactiveCouchbaseOperations, Mono<?>> callback) { return doInTransaction(tryCount, callback); }

Ah, dang. That's annoying. Is that a fundamental issue with Spring @transactional or is it limited to our implementation? If it's the former, I'm not sure we should be calling it out in our docs - as elsewhere, us mentioning Spring limitations a) makes us look bad and b) is info the user should get from Spring's own docs, I think.

programmatix · 2022-07-22T09:37:46Z

src/main/asciidoc/transactions.adoc

+    Person person = rxCBTmpl.insertById(Person.class).inCollection(cName).one(WalterWhite).block();
+    Mono<Person> result = rxCBTmpl.findById(Person.class).one(person.id())
+        .flatMap(p -> rxCBTmpl.replaceById(Person.class).one(p.withFirstName("Walt")))
+        .as(txOperator::transactional);


No no no no please let's not have this .as() :) Hopefully it's not actually in the API and just a docs oversight?

If not, well...

As we discussed a lot on Slack, I feel this adds a lot of confusion to the user. It's going to show up to them all the time on Intellisense. They're going to wonder in what contexts they need to be calling it - inside @transactional? Inside a regular SDK transaction? We're polluting our API for CouchbaseTransactionalOperator which we don't even need..

Plus, it's such a footgun, they can forget it so easily.

And, most importantly, it can't support retries.

For my reference, mention of .as() is now resolved.
And hopefully isn't in the API itself...

programmatix · 2022-07-22T09:42:39Z

src/main/asciidoc/transactions.adoc


-.Transaction Conversion on Read
+== Transactions with Only the SDK


Oh wow, this sections is very barebones :)

I feel this should be our best seller. We made the regular transactions API support all Spring operations too - that's really cool! I think this should be the main way that users interact with transactions in Spring. We should have this front and centre, and mention @transactional second - IMO.

Can this section be fleshed out a lot more? I'd like to see a good description section, and examples including:

Doing correct error handling of TransactionCommitAmbiguousException and TransactionFailedException (see the docs)

Mixing the normal transactions API with Spring repo and template methods.

Can we not call it "Only the SDK", it just sounds a bit negative :)
Maybe "Using the Transactions SDK Directly" ? Or something, I dunno, naming things is hard.

Yes - it's brief. Spring Data frowns upon anything that is specific to a technology. Hopefully users will go to the Couchbase Transactions how-to link to see everything.

I changed it to "directly".
I meant "only" to mean "Independently" - not "merely" :)

Sure, we certainly don't want to C&P the transaction docs here or anything. But I think showing the Spring-specific portions is very useful - e.g. being able to use Spring operations directly inside the lambda.

I know you didn't intend any slight :) It just would come across to the reader as a bit negative, IMO. Thanks for changing it.

I still feel like we could talk this section up a bit, it's incredibly barebones. But I guess we just disagree on this one.

I looked at what Maria did with the main transactions documentation. I want her to give this a going-over when she gets a chance. (also the whole spring-data-couchbase documentation - it's all very ... minimal).

Yeah that's a great idea, Maria will likely have some good suggestions.

src/main/asciidoc/transactions.adoc

programmatix · 2022-07-22T09:46:44Z

README.adoc

+  docs=`pwd`/target/site/reference/html
+  cd /tmp 
+  # see https://docs.github.com/en/pages/getting-started-with-github-pages/creating-a-github-pages-site
+  git clone [email protected]:mikereiche/staged.git -b gh-pages


Probably don't want your name here?

programmatix · 2022-07-22T09:55:04Z

src/main/asciidoc/transactions.adoc

+named com.example.CustomExceptionV2 (an exception in the same package as
+CustomException but with an additional suffix) or an exception named
+com.example.CustomException$AnotherException (an exception declared as
+a nested class in CustomException).


This is a horrendous wall of text, and we're discussing the nitty gritty of rollback rules before we've shown the user a single example of @transactional in action. But as above I think we need to replace all this with 'don't work, soz' anyway.

programmatix · 2022-07-22T09:58:37Z

src/test/java/org/springframework/data/couchbase/domain/CapellaConnectSample.java

@@ -90,10 +90,11 @@ public static void main(String... args) {
 		}

 		ClusterEnvironment env = ClusterEnvironment.builder()
-				.securityConfig(SecurityConfig.enableTls(true)/*.trustManagerFactory(InsecureTrustManagerFactory.INSTANCE)*/)
+				//.securityConfig(SecurityConfig.enableTls(true)/*.trustManagerFactory(InsecureTrustManagerFactory.INSTANCE)*/)


Do we want this commenting left in the example?

Oops. This file shouldn't be in this branch.

programmatix · 2022-07-26T11:46:59Z

src/main/asciidoc/transactions.adoc

-
-Once it is included in your project, you need to create a single `Transactions` object. Conveniently, it can be part of
-your spring data couchbase `AbstractCouchbaseConfiguration` implementation:
+Couchbase Transactions are normally leveraged with the @Transactional operator.


What is the "@transactional operator"? Can we not call it the "@transactional annotation"?

Also as mentioned in the last review, I think we should steer people to using our normal transaction API as the first port of call. It's the most powerful - @transactional has limitations and complexities.

yes - @transactional annotation.

I'm going to leave @Transacitonal section first for the sake of spring and marketing.

Ok. We disagree on this, but it's ok, we just have different priorities.

programmatix · 2022-07-26T11:49:02Z

src/main/asciidoc/transactions.adoc

-	}
-
+// Usual Setup
+@Override public String getConnectionString() { /* ... */ }


Formatting is funky here

I reformatted this - hopefully better.

Ok - I looked at this again. What is messed up? From the actual code, I removed one level of indenting because it's all indented. And the rest is indented two spaces. The comments are at the same level as the code. And the wrapped line is indented two spaces.

Can't fully recall but I think it was at the start of the line, rather than indented as you'd expect inside a class. Looks all good now anyway.

Oh - I think I know. I copied right from the java file and removed the lines I didn't want - which resulted in the imports having no indenting and the code having indenting (since it was in a method) which looks weird in a sample.

programmatix · 2022-07-26T11:50:28Z

src/main/asciidoc/transactions.adoc

+Functioning of the  @Transactional method annotation requires 
+[start=1]
+. the configuration class to be annotated with @EnableTransactionManagement;
+. the service object with the annotated methods must be annotated with @Service and @Component;


"the service object" - is this a term that spring users will be familiar with? How about "@Service-annotated class"? Could we maybe make the first example be a concise but complete example including the @service? We're talking about things we haven't illustrated yet.

I'm trying to point out the actual object, not the class. And "the proxy object instantiated from the @Service-annotated class" is verbose and redundant. (one can have a perfectly valid @Service-annotated class, but if the object is created directly using the constructor instead letting spring create it, then @transactional won't work. (Actually it is an AOP proxy object created from the class). If folks want to know what an @service is they can refer to the spring documentation - I'm not going to give a tutorial on @service here.

Yeah I wasn't asking for a tutorial on what a service is, or anything extremely verbose like the proxy object thing. Just that the text made it clearer you were referring to a @service. The new numbering cross-referenced to the code is fine.

programmatix · 2022-07-26T11:54:45Z

src/main/asciidoc/transactions.adoc

+Functioning of the  @Transactional method annotation requires the configuration class to be annotated 
+with @EnableTransactionManagement. The service object with the annotated methods must be obtained as an @Bean either via @Autowire or
+from the application context. And the call to the method must be made from a different class than service because calling an annotated
+method from the same class will not invoke the Method Interceptor that does the transaction processing.


Ah, dang. That's annoying. Is that a fundamental issue with Spring @transactional or is it limited to our implementation? If it's the former, I'm not sure we should be calling it out in our docs - as elsewhere, us mentioning Spring limitations a) makes us look bad and b) is info the user should get from Spring's own docs, I think.

programmatix · 2022-07-26T11:55:38Z

src/main/asciidoc/transactions.adoc

+
+=== Transaction Management
+
+This annotation commonly works with thread-bound transactions managed by a


Yeah that's what I was getting at here - it's totally an implementation detail I think, no need to mention as you say.

src/main/asciidoc/transactions.adoc

programmatix · 2022-07-26T12:11:13Z

src/main/asciidoc/transactions.adoc

 ====
 [source,java]
 ----
-@Autowired
-MappingCouchbaseConverter mappingCouchbaseConverter;
+public static CouchbaseTransactionalOperator create(CouchbaseCallbackTransactionManager manager) 


Same feedback

programmatix · 2022-07-26T12:11:33Z

src/main/asciidoc/transactions.adoc

+Person person = reactiveCouchbaseTemplate.insertById(Person.class).inCollection(cName).one(WalterWhite).block();
+Flux<Person> result = txOperator.execute((ctx) -> reactiveCouchbaseTemplate.findById(Person.class).one(person.id())
+  .flatMap(p -> reactiveCouchbaseTemplate.replaceById(Person.class).one(p.withFirstName("Walt"))));
+result.blockLast();


Example formatting is a bit messy and hard to read

It's what the spring-data specified formatter gives. I tried formatting these to show the different parts of the transactions (find followed by replace in this case), but Michael complained.

That's a shame, it's still quite hard to read.

programmatix · 2022-07-26T12:13:38Z

src/main/asciidoc/transactions.adoc

-   Airline airline = new Airline("demo-airline", "at");
-   CouchbaseDocument target = new CouchbaseDocument();
-   mappingCouchbaseConverter.write(airline, target);
+== Transactions Directly with the SDK


So as in previous review

I think this should be front and centre as the Couchbase recommended way to do transactions with Spring, and mentioned right at the top. Full power, no limitations.

It's too short :) We don't want to C&P the docs but we can mention what's cool about this. We can say that you can use any Spring operations directly inside the lambda, that you can mix and match those with the regular transactional API, that sort of thing. Let's big ourselves up a bit :)

The link to the transactions doc is there.

We can say that you can use any Spring operations directly inside the lambda

Yes - I seem to have copied only the samples and lost the accompanying text. Will revisit that.

programmatix · 2022-07-26T12:15:30Z

src/main/asciidoc/transactions.adoc

Is this the end of the docs now?
There's an awful lot of feedback from before that hasn't been incorporated... including very very crucial things like talking about the retry model. Could you please take another pass on my previous review? Anything you disagree with and don't want to incorporate, please comment on it and we can iterate further.

I'll look at the review comments again.
I'll try to describe the retry model. But on the one hand there are a bunch of comments about not getting bogged down in the weeds and implementation details, and on the other hand "there's no explanation for xyz".

I mean, yes - there were niche concerns that were extremely detailed or didn't apply to us at all (it's much better now), and crucial aspects that weren't described.

programmatix · 2022-07-28T12:40:44Z

src/main/asciidoc/transactions.adoc

-Airline read = mappingCouchbaseConverter.read(Airline.class, source);
+TransactionResult result = reactiveCouchbaseTemplate.getCouchbaseClientFactory().getCluster().reactive().transactions()
+    .run(ctx -> TransactionalSupport.checkForTransactionInThreadLocalStorage().then(Mono.defer(() -> {
+      reactiveCouchbaseTemplate.insertById(Person.class).one(WalterWhite);


Shouldn't include checkForTransactionInThreadLocalStorage right?

src/main/asciidoc/transactions.adoc

Closes #1454,#1512.

mikereiche requested a review from daschl July 21, 2022 19:20

mikereiche force-pushed the datacouch_1454_transactions_documenation branch from 3ec8f6f to 998c3ee Compare July 21, 2022 20:09

daschl suggested changes Jul 22, 2022

View reviewed changes

programmatix suggested changes Jul 22, 2022

View reviewed changes

programmatix reviewed Jul 22, 2022

View reviewed changes

mikereiche force-pushed the datacouch_1454_transactions_documenation branch from 998c3ee to 44c7243 Compare July 25, 2022 21:40

mikereiche requested a review from daschl July 25, 2022 21:51

mikereiche force-pushed the datacouch_1454_transactions_documenation branch from 44c7243 to 6b8c026 Compare July 25, 2022 22:10

programmatix suggested changes Jul 26, 2022

View reviewed changes

mikereiche force-pushed the datacouch_1454_transactions_documenation branch 2 times, most recently from 57af87e to bf9edc4 Compare July 27, 2022 22:54

programmatix suggested changes Jul 28, 2022

View reviewed changes

Update transactions documentation for native transaction support.

ae31b73

Closes #1454,#1512.

mikereiche force-pushed the datacouch_1454_transactions_documenation branch from bf9edc4 to ae31b73 Compare July 28, 2022 19:29

mikereiche merged commit 28d9f8f into main Jul 28, 2022

mikereiche deleted the datacouch_1454_transactions_documenation branch February 27, 2023 18:10


		.Transaction Conversion on Read
		== Transactions with Only the SDK


		=== Transaction Management

		This annotation commonly works with thread-bound transactions managed by a

Update transactions documentation for native transaction support. #1513

Update transactions documentation for native transaction support. #1513

Uh oh!

Conversation

mikereiche commented Jul 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikereiche Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikereiche Jul 25, 2022 •

edited

Loading