Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: saga remove spring #6017

Closed
wants to merge 22 commits into from

Conversation

wt-better
Copy link
Contributor

@wt-better wt-better commented Nov 11, 2023

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

Seata saga current is strong dependency on spring. such as:

  1. code was dependency spring beans (InitializingBean ..etc)
  2. expression evaluation dependency spring el
  3. spring bean service Task dependency spring ioc

This is not conducive to saga's multilanguage building, so I has decouple saga with spring.

  1. To ensure code compatibility, make new module named 'seata-saga-spring' and move related classes(not modified path) to this module.

  2. 'seata-saga-tm' has spring dependency, this module is not necessary. First I removed this module and move class to 'seata-saga-spring' , I will sink TM's core capabilities ‘seata-saga-engine’ module later pr.

  3. 'seata-saga-engine-store' was removed, because It is the core function of the engine ,should not be separated.

  4. 'seata-saga-rm' has class named StateMachineEngineHolder, i moved to ‘seata-saga-engine’ module, and package name change, it`s not compatible. why ? I think it is inner use, users rarely use it.

so the architecture will be:

image

history architecture was:
image

@wt-better wt-better added mode: SAGA SAGA transaction mode type: feature Category issues or prs related to feature request. labels Nov 12, 2023
} else {
elContext = context.getVariables();
elContext = context.getVariable(DomainConstants.VAR_NAME_OUTPUT_PARAMS);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

需要测试下这里是否兼容

Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Merging #6017 (48d865e) into 2.x (abfafc3) will decrease coverage by 0.87%.
The diff coverage is 0.50%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6017      +/-   ##
============================================
- Coverage     49.56%   48.70%   -0.87%     
- Complexity     4748     4762      +14     
============================================
  Files           907      914       +7     
  Lines         31284    31857     +573     
  Branches       3770     3862      +92     
============================================
+ Hits          15506    15515       +9     
- Misses        14246    14811     +565     
+ Partials       1532     1531       -1     
Files Coverage Δ
...aga/engine/expression/spel/SpringELExpression.java 0.00% <ø> (ø)
.../engine/invoker/impl/SpringBeanServiceInvoker.java 0.00% <ø> (ø)
...src/main/java/io/seata/saga/util/ResourceUtil.java 66.66% <100.00%> (ø)
...ser/impl/CompensateSubStateMachineStateParser.java 66.66% <ø> (ø)
...a/statelang/parser/impl/SubStateMachineParser.java 90.90% <ø> (ø)
...tatelang/parser/utils/DesignerJsonTransformer.java 54.65% <ø> (ø)
...boot/autoconfigure/SeataSagaAutoConfiguration.java 0.00% <0.00%> (ø)
...eata/saga/tm/DefaultSagaTransactionalTemplate.java 0.00% <0.00%> (ø)
...ine/expression/spel/SpringELExpressionFactory.java 0.00% <0.00%> (ø)
...seata/saga/engine/config/DbStateMachineConfig.java 0.00% <0.00%> (ø)
... and 3 more

... and 3 files with indirect coverage changes

Copy link
Member

@ptyin ptyin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @wt-better have done a really fantastic refactoring job! The changes are HHHUGE though ;)

@@ -373,4 +372,22 @@ public static boolean hasUpperCase(String str) {
return false;
}

public static boolean hasText(String str) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think hasText and containsText are redundant, as it completely identical to isNotBlank.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this named with history spring naming, for less change. It can also be changed to ‘ containsText’

Comment on lines 288 to 301
/**
* Init StateLogStore by subClass
*
* @return StateLogStore
*/
public abstract StateLangStore initStateLogStoreStore() throws Exception;

/**
* Init StateLogStore by subClass
*
* @return StateLogStore
*/
public abstract StateLogStore initStateLogStore() throws Exception;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont get the point of having these two abstract methods. I see the only implementation to these two methods merely return null.

P.S., there is a typo. Should change initStateLogStoreStore to initStateLangStore in method signature and code comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we remove seata-saga-engine-store module, and replace it to seata-saga-engine/store/db. I'd like to know the underlying reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. seata-saga-engine-store module was engine strongly relevant.
  2. less module can reduce complexity

@@ -15,6 +15,7 @@
*/
package io.seata.saga.engine.pcext.handlers;

import io.seata.common.util.StringUtils;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import is misplaced.

@@ -15,6 +15,7 @@
*/
package io.seata.saga.engine.pcext.interceptors;

import io.seata.saga.engine.pcext.handlers.ServiceTaskStateHandler;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more misplaced imports, please check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this file is copied from spring-framework, right? I wonder if there is a copyright issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was fixed to jre uuid

@@ -52,14 +50,13 @@ public void testParser() throws IOException {
System.out.println(fastjsonOutputJson);

Assertions.assertEquals("simpleTestStateMachine", stateMachine.getName());
Assertions.assertTrue(stateMachine.getStates().size() > 0);
Assertions.assertFalse(stateMachine.getStates().isEmpty());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid these non-related changes so to reduce CR complexity

@funky-eyes funky-eyes changed the title Feature/saga remove spring feature: saga remove spring Dec 23, 2023
…ring

# Conflicts:
#	common/src/main/java/io/seata/common/util/StringUtils.java
#	saga/seata-saga-engine-store/pom.xml
#	saga/seata-saga-engine/src/main/java/io/seata/saga/engine/config/AbstractStateMachineConfig.java
#	saga/seata-saga-engine/src/main/java/io/seata/saga/engine/repo/StateMachineRepository.java
#	saga/seata-saga-engine/src/main/java/io/seata/saga/engine/store/db/DbStateLogStore.java
#	saga/seata-saga-spring/src/test/java/io/seata/saga/util/ResourceUtilTests.java
#	saga/seata-saga-statelang/src/test/java/io/seata/saga/statelang/parser/StateParserTests.java
#	saga/seata-saga-tm/pom.xml
#	test/src/test/resources/saga/spring/statemachine_engine_db_mockserver_test.xml
#	test/src/test/resources/saga/spring/statemachine_engine_db_test.xml
#	test/src/test/resources/saga/spring/statemachine_engine_test.xml
@wt-better wt-better closed this Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mode: SAGA SAGA transaction mode type: feature Category issues or prs related to feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants