Compatible with CompleteFuture asynchronous programming

Motivation:

As shown in the following link, the Sofarpc part of the code is written based on an older version of jdk, which is not compatible with the usage of jdk8 CompleteFuture. In order to make the framework more compatible with asynchronous programming, I have modified the corresponding ResponseFuture content

https://github.com/sofastack/sofa-rpc/issues/945#issuecomment-1495839924

Modification:

My idea is that ResponseFuture directly inherits CompleteFuture, as it already inherits Future and CompletionStage, and there is no need to implement classes to override these methods in the future

Result:

Fixes #.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ad29f00) 72.01% compared to head (09a51f7) 72.06%. Report is 30 commits behind head on master.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1323 +/- ## ============================================ + Coverage 72.01% 72.06% +0.04% - Complexity 783 784 +1 ============================================ Files 416 416 Lines 17659 17663 +4 Branches 2752 2752 ============================================ + Hits 12718 12729 +11 + Misses 3538 3529 -9 - Partials 1403 1405 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

I wrote a single test, but I won't print the corresponding results

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

This pull request titled "Compatible with CompletableFuture asynchronous programming" consists of several patches making changes to the com.alipay.sofa.rpc.message package and modifying the maven.yml GitHub workflow file. The primary intent is to make the ResponseFuture class compatible with Java 1.8's CompletableFuture asynchronous programming.

Potential issues and errors:

Backward compatibility for ResponseFuture and AbstractResponseFuture might be affected due to transitioning from an interface to an abstract class.
The rationale behind the removal of the generic type V from ResponseFuture's method signatures is unclear, which might introduce type safety issues and impact existing code.
Some code formatting changes do not follow standard Java conventions and may lead to inconsistencies in the codebase.
Code changes in some patches are primarily concerned with formatting or stylistic preferences, which do not provide a clear indication of how the intent of compatibility with CompletableFuture is achieved.
Modifications to the GitHub Actions workflow (maven.yml) may introduce issues related to automatic build triggers, package deployment, and caching.

Most important findings:

Several changes made in the classes ResponseFuture, AbstractResponseFuture, and HystrixResponseFuture attempt to implement compatibility with CompletableFuture.
Method signatures in ResponseFuture and other classes are updated, improving the usage of generics and ensuring better type safety.
Support for timeout functionality is included in the await() method of the AbstractResponseFuture class.
The PR contains several changes to the maven.yml GitHub workflow, including build triggers, the build process, test execution, and Codecov integration.

Overall, the PR contains changes aimed at making the ResponseFuture class compatible with Java 1.8's CompletableFuture. However, certain modifications could raise potential issues related to backward compatibility, type safety, code clarity, and the GitHub Actions workflow. A thorough review of each patch is advised to ensure the desired compatibility is achieved, and potential issues are addressed before merging the changes.

Details

Commit 19cb9820d9defa612bb0c84ae82a7cc5b34ea3e6

This patch modifies two files within the com.alipay.sofa.rpc.message package to make the ResponseFuture class compatible with Java 1.8's CompletableFuture asynchronous programming.

Key changes:

AbstractResponseFuture class now extends ResponseFuture instead of implementing it. Some method signatures are also updated accordingly.
ResponseFuture class is changed from being an interface to an abstract class and now extends CompletableFuture.

Potential problems:

The change from an interface to an abstract class in ResponseFuture might break backward compatibility for some users, as they cannot implement the interface anymore.
The modifications in the AbstractResponseFuture class might also affect existing subclasses that implement the old ResponseFuture interface.

Commit f5eb764ae3a2588d9c311c0a3b604cbb01270d8e

This GitHub patch is a part of a Pull Request titled "Compatible with CompletableFuture asynchronous programming". The changes in the patch are as follows:

In AbstractResponseFuture.java, there are formatting changes, which adjust the alignment of the code.
In the same file, a for (;;) loop's formatting is changed to for (; ;).
The patch removes an unused import, import java.util.concurrent.Future;, from ResponseFuture.java.

Key findings:

The main goal of making the code compatible with Java 1.8 CompletableFuture asynchronous programming is not visible in this patch. It seems like formatting changes and removal of an unused import.
There are no significant code changes in this patch apart from formatting adjustments and removal of an unused import statement.

Potential problems:

Since the main goal of the PR is not visible in the patch, there could be further changes that need to be made in other patches to achieve compatibility with Java 1.8 CompletableFuture.
There is no logical modification done in this patch. This makes it difficult to assess if the changes are relevant or necessary for achieving compatibility with Java 1.8 CompletableFuture asynchronous programming.

Commit 682ac4005431eadc4ad1fc8f435800b375b8d590

This patch introduces changes in the ResponseFuture and HystrixResponseFuture classes that aim to make the codebase compatible with Java 1.8's CompletableFuture asynchronous programming. The key changes are as follows:

Update method signatures in ResponseFuture class:
- Change the return type of addListeners() and addListener() methods to ResponseFuture<V> from ResponseFuture.
Modify inheritance in HystrixResponseFuture class:
- Change HystrixResponseFuture to extend ResponseFuture instead of implementing it.

Potential Problems:

Generics missing from SofaResponseCallback:
- The patch does not add any generic type information to SofaResponseCallback in the method signatures, which might lead to warnings or errors during compilation or runtime.
Unclear compatibility with CompletableFuture:
- The changes do not visibly demonstrate how these modifications ensure compatibility with Java 1.8's CompletableFuture. Further investigation of the full codebase may be necessary to understand the intent and implications of these changes.
Incomplete implementation:
- This patch only contains a few updates and does not cover the complete implementation of CompletableFuture features in the codebase.

Commit 6fe8c4a8429cf878b44c0343a4793da022d0c432

The key changes in this patch are related to the ResponseFuture class in the com.alipay.sofa.rpc.message package. The patch modifies two method signatures:

public abstract ResponseFuture<V> addListeners(List<SofaResponseCallback> sofaResponseCallbacks); is changed to public abstract ResponseFuture addListeners(List<SofaResponseCallback> sofaResponseCallbacks);
public abstract ResponseFuture<V> addListener(SofaResponseCallback sofaResponseCallback); is changed to public abstract ResponseFuture addListener(SofaResponseCallback sofaResponseCallback);

The changes remove the generic type V from the return type of both methods. The commit message indicates that these changes are supposed to make the ResponseFuture class compatible with Java 1.8's CompletableFuture asynchronous programming.

Potential problems with this patch:

Removing the generic type may lead to type safety issues, as the type of the returned ResponseFuture is now unknown.
Existing code that relies on the generic type information may raise compile-time errors or require updates to accommodate this change.
The commit message and patch title do not provide sufficient information about how this change will make the class compatible with CompletableFuture, so the rationale for the change is unclear. This may make it difficult for other developers to review and understand the difference it would make.

Commit 964db3c2fcc11abacb8e40bc8adbebf4add19629

This patch makes a change to the AbstractResponseFuture class in the com.alipay.sofa.rpc.message package. The change is relatively small and includes only a formatting change, adding an extra space before the class name.

Summary of key changes:

Added an extra space before the class name in the AbstractResponseFuture class declaration.

Given that this is a formatting change, there are no direct functional impacts or potential problems introduced by the patch. However, it is worth noting that the change does not follow standard Java formatting conventions and might cause inconsistencies in the codebase's formatting. It is recommended to revert this change or apply proper formatting to the entire codebase to maintain consistency.

Commit 7badd318d2edc53ded7ed851f6cb9f0e945a1f4b

This patch makes several key changes to the maven.yml GitHub workflow file:

The new workflow is set to run only on workflow_dispatch events, which means it will only run when manually triggered. Previously, it was set to run on pushes and pull requests to the 'master' branch.
The steps for setting up JDK 1.8 and building the project with Maven have been updated to use the latest version of the actions/checkout@v3 and actions/setup-java@v3 actions.
Added a cache step for improved workflow execution time using the cache: maven option in the actions/setup-java@v3 action.
Reformatted the workflow for better readability and separation of steps.
Added a new release job that is dependent on the build job, which sets up JDK 8, sets Maven credentials, and handles GPG keys to sign the build artifacts. The release step runs mvn --batch-mode deploy to deploy the project to a Maven repository.

Potential problems:

The removal of branch triggers for push and pull requests means that the workflow will no longer run automatically upon pushes or pull requests to the 'master' branch. This might not be desired, especially for an open-source project where continuous integration is a common requirement.
The on: workflow_dispatch:, syntax may cause issues as it should be on: workflow_dispatch.
There are no test steps in the release job, which means no tests will run during the release process. Depending on project requirements, this could be desired or undesired.

Commit e1d0a9b1d4c58ffd3af39ca22f39b616e6bcf2db

This patch contains updates to AbstractResponseFuture.java within the com.alipay.sofa.rpc.message package. The key changes are:

The code has been reformatted, which includes changes to the amount of whitespace used for alignment. This doesn't affect the functionality but could be a stylistic preference of the developer.
The await method now accepts a timeout and unit as parameters to control the amount of time to wait. Previously, it would block indefinitely.
There is a new await0 private method that await uses, which accepts timeoutNanos and interruptable as parameters to control the amount of time to wait and whether the wait can be interrupted. This utilizes a while loop to repeatedly attempt to wait for the specified amount of nanoseconds, handling the interruption if enabled.
The for loop has been changed from for (; ; ) to for (;;), which is a minor stylistic change that doesn't impact functionality.

Potential problems:

Code readability: The code reformatting can make it harder for other developers to read, especially if they're used to the existing code style.
Compatibility: Changing the await method's signature means it could potentially break compatibility with other parts of the codebase that rely on this method.

Overall, while these changes improve the functionality of the AbstractResponseFuture class by introducing timeouts for the await method, they could cause potential problems in terms of code readability and compatibility.

Commit b7944a22fdfb498cda13a1f4631976292a46a774

This pull request makes several changes to the maven.yml file in a GitHub Actions workflow for building and testing a Java project with Maven. The key changes are as follows:

The workflow now triggers on push and pull_request events to the master branch, as well as on manual workflow dispatch events.
The jobs section is simplified by removing the release job and combining its functionality with the build job.
The workflow now uses actions/checkout@v2 instead of actions/checkout@v3.
JDK 8 is renamed to JDK 1.8.
A new Test with Maven step is added, which runs tests using the ./mvnw package -Pci-test Maven command.
The Codecov step is added using the codecov/codecov-action@v1 action.

Potential problems:

The release job has been removed, and the corresponding Maven deployment settings are missing. This means that package deployment is no longer handled by this workflow, and if deployment is necessary, it should be reintroduced in a separate job or another workflow.
The change from actions/checkout@v3 to actions/checkout@v2 might be unintentional or could lead to issues if specific features from v3 are needed. It's important to verify that v2 is sufficient for the project's needs.
The cache settings for the release job are missing, which could affect build times if the build process relies on cached Maven artifacts. If caching is essential, the cache settings should be added back to the build job.
Renaming JDK 8 to JDK 1.8 might be confusing, as it implies a different version. It's better to stick with the standard naming convention (JDK 8) to avoid confusion.

Commit 87311b356e25cfa81f5f53f880908a9a110a689f

The patch contains a single modified file, AbstractResponseFuture.java, from the core API package. There are no functional changes, and key modifications include:

Formatting: There are three changes to adjust the appearance of the code.
- The whitespace between class and AbstractResponseFuture<V> keywords has been reduced from two spaces to one.
- The throws InterruptedException line in the await method has been indented by 4 more spaces.
- A newline has been removed from the end of the file, causing a warning about not having a newline at the end.

Potential problems:

No functional changes: As this patch does not introduce any functional changes, it is unlikely to cause any issues or improvements to the software.
No newline at the end of the file: Removing the newline character at the end of the file might not follow the coding standards in some cases and can potentially cause problems while merging with other patches.

Commit 8c06b4e158b318f477d74a7a78eecbaf0e925574

The GitHub patch modifies the AbstractResponseFuture.java file. The key changes are:

Only one line has been changed in the entire patch. The change simply moves the "throws InterruptedException" from the same line as the function declaration to a new line.

There don't seem to be any significant functional changes or potential problems introduced by this minor formatting change.

Commit aa9a1be5ce4bb03172c61c5e416a3a1b368edf08

The patch introduces changes to the AsyncChainTest.java file, which is part of a test suite for asynchronous programming.

Key Changes:

Adds a new test case in the testAll() method.
The new test case demonstrates the usage of Java's CompletableFuture by attaching a callback function to a future.
The callback function logs the result from the CompletableFuture.

Potential Problems:

The LOGGER variable might not be properly initialized or declared. If it isn't, this would result in a compilation error.
This test case is added to the existing testAll() method, so it can potentially interfere with other test cases in the same method. It's generally a better practice to separate test cases into different methods.
There is no assertion or verification in the new test case to check if the CompletableFuture result is as expected. This might lead to false positives if the feature under test is not working correctly.

Commit f8250df2d1088737b1e8975d539f858dec1ad931

The patch includes changes to one file, .github/workflows/maven.yml.

Key Changes:

There is a minor modification to the "Codecov" step, which updates the action to use the codecov/codecov-action@v1. There is no change in the version of the codecov action being used.

Potential Problems:

There is no newline at the end of the file. This can potentially cause issues with some compilers, build tools, or even parsers not processing the file properly.

Overall, the changes appear relatively insignificant, and the only possible problem identified is the missing newline at the end of the file.

Commit 0a1b40755a05d02483ee6f9751038001c8498508

This patch modifies the AsyncChainTest.java file in the test-integration module. The key changes are:

Commenting out a part of the code:
- The code that was commented out is the use of the thenAccept() method on a CompletableFuture object, which is used to perform an action when the future is completed.
- The commented out code logs the result of the future using LOGGER.

Potential problems:

Commenting out this part of the code might affect the expected logging behavior in the test, which could make debugging failures or understanding the test output more difficult.
Since this is a test-related change, it might not have any direct impact on the production code. However, it could affect the ability to catch potential issues through this test.

It is unclear why the author decided to comment out this block of code without providing an explanation or an alternative implementation. The pull request would benefit from more context or discussion with the author to understand the reasoning behind this change.

Commit 4d13a3e5da1fe2138689ac3ec279266954508b38

Summary of key changes:

The patch is modifying a test file named AsyncChainTest.java.
The main change is uncommenting two lines within the testAll() method.

Potential issues:

The patch does not introduce any logical changes, meaning it might have little impact on the functionality being tested.
Since the pull request title mentions compatibility with CompletableFuture asynchronous programming, the test file may not properly address the intended test cases to verify compatibility.
There is only one file changed in this patch; it does not provide enough context to understand if the changes are benefiting the overall project or implementing a specific feature properly.

Commit 34c4c092a5d1b606ba78f09b055d16a3c03255f6

Summary of key changes:

Wrapped the CompletableFuture's thenAccept() call within a try-catch block to handle potential exceptions during its execution.
Added error logging that captures the exception details if an error occurs during the execution of the CompletableFuture.

Potential problems:

Catching the generic Exception in the try-catch block might not be ideal. It may be better to catch more specific exceptions depending on the potential issues that can arise during the execution of thenAccept().

Commit adf85aab61526791ac878deb769fd60158fc2a7e

The patch modifies code in two test files: AsyncChainTest.java and Http2ClearTextHessianTest.java

Key changes:

In AsyncChainTest.java, the CompletableFuture handling section is removed which includes the thenAccept() block and the error-catching block. The removal of this block means that the test case no longer checks for CompletableFuture results and errors.
In Http2ClearTextHessianTest.java, a new thenAccept() block is added to handle CompletableFuture results by logging the result, after calling response = future.get();. Additionally, an error logging statement is added in the existing catch block.

Potential problems:

By removing CompletableFuture handling in AsyncChainTest.java, the test case loses an important part of validation. This could lead to false pass or fail results.
With the new addition of the thenAccept() block in Http2ClearTextHessianTest.java, there might be a possibility of duplicate logging of CompletableFuture result, as the result is already logged when calling response = future.get();.
There appears to be a minor inconsistency in the position of the CompletableFuture handling section between the two test cases. While it is inside the try block in Http2ClearTextHessianTest.java, it was earlier outside the try block in AsyncChainTest.java. This may not result in any functional issues, but it may cause inconsistencies in how similar code is organized in different parts of the project.

Commit f7cda08c6648bd25f502eed0e54786e3475b258e

This GitHub patch belongs to a Pull Request titled "Compatible with CompleteFuture asynchronous programming" and introduces two changes:

In .github/workflows/maven.yml, the newline at the end of the file is added back, changing the following line:
```
-      uses: codecov/codecov-action@v1
\ No newline at end of file
+      uses: codecov/codecov-action@v1
```
In core/api/src/main/java/com/alipay/sofa/rpc/message/AbstractResponseFuture.java, the newline at the end of the file is added back, changing the following line:
```
-}
\ No newline at end of file
+}
```

Both of these changes address the same issue - ending a file with a newline character. These changes don't have any impact on the functionality of the project.

Commit 8efc6246942aa226d4629463f8bc3760f00cedb7

The patch includes changes across two files: FutureTest.java and Http2ClearTextHessianTest.java.

Key Changes:

In FutureTest.java, the patch adds a thenAccept block for the CompletableFuture instance accessible through RpcInvokeContext.getContext().getFuture(). This block logs the CompletableFuture result when it becomes available.
In Http2ClearTextHessianTest.java, the same thenAccept block has been removed.

Potential Problems:

Inconsistency in handling CompletableFuture results: The thenAccept block has been added in FutureTest.java but removed from Http2ClearTextHessianTest.java. This could lead to an inconsistent approach to handling CompletableFuture results across different test cases.
Removing the thenAccept block in Http2ClearTextHessianTest.java might affect the expected output of the test, possibly leading to false positives or negatives in the test results.

Commit 34025eee11c262d09407977be569662acbf78ad0

The patch introduces a small change in the GitHub Actions workflow for a Maven project. The key change is the addition of a newline at the end of the 'maven.yml' file.

Summary of changes:

1 file changed, 1 insertion(+)
Inserted a newline at the end of '.github/workflows/maven.yml'

Potential problems:

No real functionality or improvement is added to the codebase.
The commit message is not clear, which may cause confusion during code review or when browsing the commit history.

Commit 255393c22c7bc5751a07668b3c60d290c1ff5550

This patch makes a change in a single test file (Http2ClearTextHessianTest.java) within the com.alipay.sofa.rpc.transport.http package. The patch removes one line of code in the testHessian() method:

LOGGER.error("CompletableFuture error: {}", e);

Key Changes:

Removal of a log statement used to log errors related to CompletableFuture handling.

Potential problems:

Depending on the purpose of the log statement, removing it might make it harder to debug CompletableFuture errors in this test method in the future. However, since the error is still printed using e.printStackTrace();, debugging might still be possible, but it may not appear in the standard log output.

Commit 4452ae12cb954be4cbe1b78d661d539abdefaef1

Summary of Key Changes:

The patch adds a new Logger class import in the FutureTest.java file.
A new private static final Logger object named LOGGER is created using the LoggerFactory.getLogger(ActivelyDestroyTest.class) method.

Potential Problems:

The LoggerFactory import and logger object creation seem to be correctly implemented. However, there are no actual logger usage or logging calls in the pull request, making it unclear how it would enhance the code.
The patch title "Compatible with CompleteFuture asynchronous programming" does not match the content of this specific patch. The patch only adds a logger and does not seem to make any changes related to CompletableFuture or asynchronous programming. This can cause confusion when reviewing patches or tracking changes in the version control system.

If possible, the patch author should provide more context or an explanation on how this specific change relates to CompletableFuture and how the logger will be used in the test class. The subsequent patches should also be reviewed to ensure they do not introduce any issues in the project.

Commit 945693292b6aaf20d793f162cc96dd84da9a2cbe

This patch introduces a change in the "FutureTest.java" file of the "test-integration" package. The key change made in this patch is updating the class used for instantiating the LOGGER field.

Key Changes:

The LOGGER field instantiation has been updated from LoggerFactory.getLogger(ActivelyDestroyTest.class) to LoggerFactory.getLogger(FutureTest.class).

Potential Problems:

This change seems minimal and not likely to cause any potential issues. The patch correctly assigns the LOGGER to the correct class (FutureTest), which should have been done initially. This change ensures that the logging will be done under the appropriate context.

Commit 63478bc5d617a3e4258d46f4c9b2e255e5f81521

This patch introduces changes in two test files in a GitHub project. The key changes are:

Updating a log message in the FutureTest class.
Adding a new Http2ClearTextHessianTest class with the following changes:
- Importing two new classes: Logger and LoggerFactory
- Adding a private static Logger instance
- Implementing a testHessian() method that incorporates the following changes:
  - Using a CompletableFuture callback with thenAccept() to log the result inside the testHessian() method

The major concern with this patch is the code inside the Http2ClearTextHessianTest class. Although it adds functionality to test Hessian support over HTTP/2, the test method implementation is not very straightforward and could be improved for better readability and understanding.

There might also be potential issues with exception handling, as the callback for the CompletableFuture is not handling any exceptions, which could lead to unhandled exceptions being thrown during the execution of the test.

Commit 57a1ecc17b3353979e96aa04779eb8079b5c79c5

Summary of key changes:

The AbstractResponseFuture class now extends CompletableFuture<V> and implements ResponseFuture<V> instead of directly extending ResponseFuture<V>.
The ResponseFuture class is changed from an abstract class to an interface and now extends Future<V> and CompletionStage<V>.

Potential problems:

The change in inheritance may cause issues in existing code where AbstractResponseFuture was expected to extend ResponseFuture directly.
The change of ResponseFuture to an interface could cause issues if any existing code was expecting it to be an abstract class, requiring updates to classes that implement this interface.
Since the ResponseFuture now implements Future<V> and CompletionStage<V>, it may introduce incompatibility with other libraries and frameworks expecting it to be a CompletableFuture<V> or other specific implementation.

Commit db141302aa15aba55a310a93306705f4de5ee144

The following is a summary of the key changes and findings:

Two test files have been updated: RpcInternalContextTest.java and InvocationStatDimensionStatTest.java.
A new private class MyResponseFuture, which extends AbstractResponseFuture, has been introduced in both test files.
The implementation of the test cases in both files has been refactored, replacing the anonymous ResponseFuture classes with instances of the new MyResponseFuture class.

Potential problems:

While this change does not introduce any new logic, it is important to ensure that the new MyResponseFuture class has the correct implementation to avoid impacting the test cases' validity.
The MyResponseFuture class seems to have several methods returning null, which might not be the best practice and can lead to NullPointerException in certain scenarios. Consider following the guidelines for handling nulls or providing a more specific implementation for these methods.
The commit message and the Pull Request title do not provide enough context and clarity on the purpose of this change. Consider providing a more descriptive and informative commit message and Pull Request title to help other developers understand the changes.

In conclusion, the changes mostly consist of refactoring the test cases by introducing a new MyResponseFuture class, which extends AbstractResponseFuture. There are no major changes to the overall logic, but it is essential to ensure that the implementations do not introduce unintended behavior.

Commit b85bb7cc694bd68cf8b192707f778b071d0eb2bb

The key change in this GitHub patch is modifying the HystrixResponseFuture class to extend CompletableFuture, rather than implementing the ResponseFuture interface.

The changes can be summarized as follows:

The HystrixResponseFuture class now extends CompletableFuture instead of ResponseFuture.
The patch removes the java.util.concurrent.* import lines for ExecutionException, Future, TimeUnit, and TimeoutException.

Potential problems include:

Compatibility issue: Changing the HystrixResponseFuture to extend CompletableFuture could impact other parts of the codebase that rely on or expect HystrixResponseFuture to implement the ResponseFuture interface.
Unnecessary import removals: Removing imports for ExecutionException, Future, TimeUnit, and TimeoutException could lead to compilation issues if there are usages of these classes within HystrixResponseFuture that were not removed in this patch.

Commit 9c793f55aa44e984624f16440e91973ab677ad6b

This patch is a part of a larger Pull Request focused on making the software compatible with CompleteFuture asynchronous programming.

Key Changes:

The patch modifies the InvocationStatDimensionStatTest.java file within the "fault-tolerance" module.
It adds 6 new lines of code to implement the get(long timeout, TimeUnit unit) method for a class, which is most likely a HystrixResponseFuture implementation.
```
@Override
public String get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException {
   return null;
}
```

Potential Problems:

The newly added get(long timeout, TimeUnit unit) method returns null, which may not be the correct or intended behavior. A proper implementation should be provided to avoid null pointer exceptions or unexpected behavior when using this method with CompletableFuture.async.

Commit 6800b15dda1419b963a237a3775ab3acd6391d38

This patch is for a test file in the fault-tolerance module of a software project. The key changes include:

Fixing the code formatting for the get(long timeout, TimeUnit unit) method declaration by moving the TimeoutException to the previous line.
Modifying the prepareInvokeContext() method by slightly changing the instantiation of MyResponseFuture and other indents.

It seems to be a simple formatting fix with no major logical changes. However, it is essential to ensure that the indentation changes do not cause any issues or unintended consequences in the code execution. Also, the commit message is not very clear and informative, but it might not be a significant concern if the patch is part of a series of related changes.

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

This pull request titled "Compatible with CompletableFuture asynchronous programming" consists of several patches making changes to the com.alipay.sofa.rpc.message package and modifying the maven.yml GitHub workflow file. The primary intent is to make the ResponseFuture class compatible with Java 1.8's CompletableFuture asynchronous programming.

Potential issues and errors:

Backward compatibility for ResponseFuture and AbstractResponseFuture might be affected due to transitioning from an interface to an abstract class.

The rationale behind the removal of the generic type V from ResponseFuture's method signatures is unclear, which might introduce type safety issues and impact existing code.

Some code formatting changes do not follow standard Java conventions and may lead to inconsistencies in the codebase.

Code changes in some patches are primarily concerned with formatting or stylistic preferences, which do not provide a clear indication of how the intent of compatibility with CompletableFuture is achieved.

Modifications to the GitHub Actions workflow (maven.yml) may introduce issues related to automatic build triggers, package deployment, and caching.

Most important findings:

Several changes made in the classes ResponseFuture, AbstractResponseFuture, and HystrixResponseFuture attempt to implement compatibility with CompletableFuture.

Method signatures in ResponseFuture and other classes are updated, improving the usage of generics and ensuring better type safety.

Support for timeout functionality is included in the await() method of the AbstractResponseFuture class.

The PR contains several changes to the maven.yml GitHub workflow, including build triggers, the build process, test execution, and Codecov integration.

Overall, the PR contains changes aimed at making the ResponseFuture class compatible with Java 1.8's CompletableFuture. However, certain modifications could raise potential issues related to backward compatibility, type safety, code clarity, and the GitHub Actions workflow. A thorough review of each patch is advised to ensure the desired compatibility is achieved, and potential issues are addressed before merging the changes.

Details

Commit 19cb9820d9defa612bb0c84ae82a7cc5b34ea3e6

This patch modifies two files within the com.alipay.sofa.rpc.message package to make the ResponseFuture class compatible with Java 1.8's CompletableFuture asynchronous programming.

Key changes:

AbstractResponseFuture class now extends ResponseFuture instead of implementing it. Some method signatures are also updated accordingly.

ResponseFuture class is changed from being an interface to an abstract class and now extends CompletableFuture.

Potential problems:

The change from an interface to an abstract class in ResponseFuture might break backward compatibility for some users, as they cannot implement the interface anymore.

The modifications in the AbstractResponseFuture class might also affect existing subclasses that implement the old ResponseFuture interface.

Commit f5eb764ae3a2588d9c311c0a3b604cbb01270d8e

This GitHub patch is a part of a Pull Request titled "Compatible with CompletableFuture asynchronous programming". The changes in the patch are as follows:

In AbstractResponseFuture.java, there are formatting changes, which adjust the alignment of the code.

In the same file, a for (;;) loop's formatting is changed to for (; ;).

The patch removes an unused import, import java.util.concurrent.Future;, from ResponseFuture.java.

Key findings:

The main goal of making the code compatible with Java 1.8 CompletableFuture asynchronous programming is not visible in this patch. It seems like formatting changes and removal of an unused import.

There are no significant code changes in this patch apart from formatting adjustments and removal of an unused import statement.

Potential problems:

Since the main goal of the PR is not visible in the patch, there could be further changes that need to be made in other patches to achieve compatibility with Java 1.8 CompletableFuture.

There is no logical modification done in this patch. This makes it difficult to assess if the changes are relevant or necessary for achieving compatibility with Java 1.8 CompletableFuture asynchronous programming.

Commit 682ac4005431eadc4ad1fc8f435800b375b8d590

This patch introduces changes in the ResponseFuture and HystrixResponseFuture classes that aim to make the codebase compatible with Java 1.8's CompletableFuture asynchronous programming. The key changes are as follows:

Update method signatures in ResponseFuture class:

Change the return type of addListeners() and addListener() methods to ResponseFuture<V> from ResponseFuture.

Modify inheritance in HystrixResponseFuture class:

Change HystrixResponseFuture to extend ResponseFuture instead of implementing it.

Potential Problems:

Generics missing from SofaResponseCallback:

The patch does not add any generic type information to SofaResponseCallback in the method signatures, which might lead to warnings or errors during compilation or runtime.

Unclear compatibility with CompletableFuture:

The changes do not visibly demonstrate how these modifications ensure compatibility with Java 1.8's CompletableFuture. Further investigation of the full codebase may be necessary to understand the intent and implications of these changes.

Incomplete implementation:

This patch only contains a few updates and does not cover the complete implementation of CompletableFuture features in the codebase.

Commit 6fe8c4a8429cf878b44c0343a4793da022d0c432

The key changes in this patch are related to the ResponseFuture class in the com.alipay.sofa.rpc.message package. The patch modifies two method signatures:

public abstract ResponseFuture<V> addListeners(List<SofaResponseCallback> sofaResponseCallbacks); is changed to public abstract ResponseFuture addListeners(List<SofaResponseCallback> sofaResponseCallbacks);

public abstract ResponseFuture<V> addListener(SofaResponseCallback sofaResponseCallback); is changed to public abstract ResponseFuture addListener(SofaResponseCallback sofaResponseCallback);

The changes remove the generic type V from the return type of both methods. The commit message indicates that these changes are supposed to make the ResponseFuture class compatible with Java 1.8's CompletableFuture asynchronous programming.

Potential problems with this patch:

Removing the generic type may lead to type safety issues, as the type of the returned ResponseFuture is now unknown.

Existing code that relies on the generic type information may raise compile-time errors or require updates to accommodate this change.

The commit message and patch title do not provide sufficient information about how this change will make the class compatible with CompletableFuture, so the rationale for the change is unclear. This may make it difficult for other developers to review and understand the difference it would make.

Commit 964db3c2fcc11abacb8e40bc8adbebf4add19629

This patch makes a change to the AbstractResponseFuture class in the com.alipay.sofa.rpc.message package. The change is relatively small and includes only a formatting change, adding an extra space before the class name.

Summary of key changes:

Added an extra space before the class name in the AbstractResponseFuture class declaration.

Given that this is a formatting change, there are no direct functional impacts or potential problems introduced by the patch. However, it is worth noting that the change does not follow standard Java formatting conventions and might cause inconsistencies in the codebase's formatting. It is recommended to revert this change or apply proper formatting to the entire codebase to maintain consistency.

Commit 7badd318d2edc53ded7ed851f6cb9f0e945a1f4b

This patch makes several key changes to the maven.yml GitHub workflow file:

The new workflow is set to run only on workflow_dispatch events, which means it will only run when manually triggered. Previously, it was set to run on pushes and pull requests to the 'master' branch.

The steps for setting up JDK 1.8 and building the project with Maven have been updated to use the latest version of the actions/checkout@v3 and actions/setup-java@v3 actions.

Added a cache step for improved workflow execution time using the cache: maven option in the actions/setup-java@v3 action.

Reformatted the workflow for better readability and separation of steps.

Added a new release job that is dependent on the build job, which sets up JDK 8, sets Maven credentials, and handles GPG keys to sign the build artifacts. The release step runs mvn --batch-mode deploy to deploy the project to a Maven repository.

Potential problems:

The removal of branch triggers for push and pull requests means that the workflow will no longer run automatically upon pushes or pull requests to the 'master' branch. This might not be desired, especially for an open-source project where continuous integration is a common requirement.

The on: workflow_dispatch:, syntax may cause issues as it should be on: workflow_dispatch.

There are no test steps in the release job, which means no tests will run during the release process. Depending on project requirements, this could be desired or undesired.

Commit e1d0a9b1d4c58ffd3af39ca22f39b616e6bcf2db

This patch contains updates to AbstractResponseFuture.java within the com.alipay.sofa.rpc.message package. The key changes are:

The code has been reformatted, which includes changes to the amount of whitespace used for alignment. This doesn't affect the functionality but could be a stylistic preference of the developer.

The await method now accepts a timeout and unit as parameters to control the amount of time to wait. Previously, it would block indefinitely.

There is a new await0 private method that await uses, which accepts timeoutNanos and interruptable as parameters to control the amount of time to wait and whether the wait can be interrupted. This utilizes a while loop to repeatedly attempt to wait for the specified amount of nanoseconds, handling the interruption if enabled.

The for loop has been changed from for (; ; ) to for (;;), which is a minor stylistic change that doesn't impact functionality.

Potential problems:

Code readability: The code reformatting can make it harder for other developers to read, especially if they're used to the existing code style.

Compatibility: Changing the await method's signature means it could potentially break compatibility with other parts of the codebase that rely on this method.

Overall, while these changes improve the functionality of the AbstractResponseFuture class by introducing timeouts for the await method, they could cause potential problems in terms of code readability and compatibility.

Commit b7944a22fdfb498cda13a1f4631976292a46a774

This pull request makes several changes to the maven.yml file in a GitHub Actions workflow for building and testing a Java project with Maven. The key changes are as follows:

The workflow now triggers on push and pull_request events to the master branch, as well as on manual workflow dispatch events.

The jobs section is simplified by removing the release job and combining its functionality with the build job.

The workflow now uses actions/checkout@v2 instead of actions/checkout@v3.

JDK 8 is renamed to JDK 1.8.

A new Test with Maven step is added, which runs tests using the ./mvnw package -Pci-test Maven command.

The Codecov step is added using the codecov/codecov-action@v1 action.

Potential problems:

The release job has been removed, and the corresponding Maven deployment settings are missing. This means that package deployment is no longer handled by this workflow, and if deployment is necessary, it should be reintroduced in a separate job or another workflow.

The change from actions/checkout@v3 to actions/checkout@v2 might be unintentional or could lead to issues if specific features from v3 are needed. It's important to verify that v2 is sufficient for the project's needs.

The cache settings for the release job are missing, which could affect build times if the build process relies on cached Maven artifacts. If caching is essential, the cache settings should be added back to the build job.

Renaming JDK 8 to JDK 1.8 might be confusing, as it implies a different version. It's better to stick with the standard naming convention (JDK 8) to avoid confusion.

Commit 87311b356e25cfa81f5f53f880908a9a110a689f

The patch contains a single modified file, AbstractResponseFuture.java, from the core API package. There are no functional changes, and key modifications include:

Formatting: There are three changes to adjust the appearance of the code.

The whitespace between class and AbstractResponseFuture<V> keywords has been reduced from two spaces to one.

The throws InterruptedException line in the await method has been indented by 4 more spaces.

A newline has been removed from the end of the file, causing a warning about not having a newline at the end.

Potential problems:

No functional changes: As this patch does not introduce any functional changes, it is unlikely to cause any issues or improvements to the software.

No newline at the end of the file: Removing the newline character at the end of the file might not follow the coding standards in some cases and can potentially cause problems while merging with other patches.

Commit 8c06b4e158b318f477d74a7a78eecbaf0e925574

The GitHub patch modifies the AbstractResponseFuture.java file. The key changes are:

Only one line has been changed in the entire patch. The change simply moves the "throws InterruptedException" from the same line as the function declaration to a new line.

There don't seem to be any significant functional changes or potential problems introduced by this minor formatting change.

Commit aa9a1be5ce4bb03172c61c5e416a3a1b368edf08

The patch introduces changes to the AsyncChainTest.java file, which is part of a test suite for asynchronous programming.

Key Changes:

Adds a new test case in the testAll() method.

The new test case demonstrates the usage of Java's CompletableFuture by attaching a callback function to a future.

The callback function logs the result from the CompletableFuture.

Potential Problems:

The LOGGER variable might not be properly initialized or declared. If it isn't, this would result in a compilation error.

This test case is added to the existing testAll() method, so it can potentially interfere with other test cases in the same method. It's generally a better practice to separate test cases into different methods.

There is no assertion or verification in the new test case to check if the CompletableFuture result is as expected. This might lead to false positives if the feature under test is not working correctly.

Commit f8250df2d1088737b1e8975d539f858dec1ad931

The patch includes changes to one file, .github/workflows/maven.yml.

Key Changes:

There is a minor modification to the "Codecov" step, which updates the action to use the codecov/codecov-action@v1. There is no change in the version of the codecov action being used.

Potential Problems:

There is no newline at the end of the file. This can potentially cause issues with some compilers, build tools, or even parsers not processing the file properly.

Overall, the changes appear relatively insignificant, and the only possible problem identified is the missing newline at the end of the file.

Commit 0a1b40755a05d02483ee6f9751038001c8498508

This patch modifies the AsyncChainTest.java file in the test-integration module. The key changes are:

Commenting out a part of the code:

The code that was commented out is the use of the thenAccept() method on a CompletableFuture object, which is used to perform an action when the future is completed.

The commented out code logs the result of the future using LOGGER.

Potential problems:

Commenting out this part of the code might affect the expected logging behavior in the test, which could make debugging failures or understanding the test output more difficult.

Since this is a test-related change, it might not have any direct impact on the production code. However, it could affect the ability to catch potential issues through this test.

It is unclear why the author decided to comment out this block of code without providing an explanation or an alternative implementation. The pull request would benefit from more context or discussion with the author to understand the reasoning behind this change.

Commit 4d13a3e5da1fe2138689ac3ec279266954508b38

Summary of key changes:

The patch is modifying a test file named AsyncChainTest.java.

The main change is uncommenting two lines within the testAll() method.

Potential issues:

The patch does not introduce any logical changes, meaning it might have little impact on the functionality being tested.

Since the pull request title mentions compatibility with CompletableFuture asynchronous programming, the test file may not properly address the intended test cases to verify compatibility.

There is only one file changed in this patch; it does not provide enough context to understand if the changes are benefiting the overall project or implementing a specific feature properly.

Commit 34c4c092a5d1b606ba78f09b055d16a3c03255f6

Summary of key changes:

Wrapped the CompletableFuture's thenAccept() call within a try-catch block to handle potential exceptions during its execution.

Added error logging that captures the exception details if an error occurs during the execution of the CompletableFuture.

Potential problems:

Catching the generic Exception in the try-catch block might not be ideal. It may be better to catch more specific exceptions depending on the potential issues that can arise during the execution of thenAccept().

Commit adf85aab61526791ac878deb769fd60158fc2a7e

The patch modifies code in two test files: AsyncChainTest.java and Http2ClearTextHessianTest.java

Key changes:

In AsyncChainTest.java, the CompletableFuture handling section is removed which includes the thenAccept() block and the error-catching block. The removal of this block means that the test case no longer checks for CompletableFuture results and errors.

In Http2ClearTextHessianTest.java, a new thenAccept() block is added to handle CompletableFuture results by logging the result, after calling response = future.get();. Additionally, an error logging statement is added in the existing catch block.

Potential problems:

By removing CompletableFuture handling in AsyncChainTest.java, the test case loses an important part of validation. This could lead to false pass or fail results.

With the new addition of the thenAccept() block in Http2ClearTextHessianTest.java, there might be a possibility of duplicate logging of CompletableFuture result, as the result is already logged when calling response = future.get();.

There appears to be a minor inconsistency in the position of the CompletableFuture handling section between the two test cases. While it is inside the try block in Http2ClearTextHessianTest.java, it was earlier outside the try block in AsyncChainTest.java. This may not result in any functional issues, but it may cause inconsistencies in how similar code is organized in different parts of the project.

Commit f7cda08c6648bd25f502eed0e54786e3475b258e

This GitHub patch belongs to a Pull Request titled "Compatible with CompleteFuture asynchronous programming" and introduces two changes:

In .github/workflows/maven.yml, the newline at the end of the file is added back, changing the following line:
-      uses: codecov/codecov-action@v1
\ No newline at end of file
+      uses: codecov/codecov-action@v1
In core/api/src/main/java/com/alipay/sofa/rpc/message/AbstractResponseFuture.java, the newline at the end of the file is added back, changing the following line:
-}
\ No newline at end of file
+}
Both of these changes address the same issue - ending a file with a newline character. These changes don't have any impact on the functionality of the project.

Commit 8efc6246942aa226d4629463f8bc3760f00cedb7

The patch includes changes across two files: FutureTest.java and Http2ClearTextHessianTest.java.

Key Changes:

In FutureTest.java, the patch adds a thenAccept block for the CompletableFuture instance accessible through RpcInvokeContext.getContext().getFuture(). This block logs the CompletableFuture result when it becomes available.

In Http2ClearTextHessianTest.java, the same thenAccept block has been removed.

Potential Problems:

Inconsistency in handling CompletableFuture results: The thenAccept block has been added in FutureTest.java but removed from Http2ClearTextHessianTest.java. This could lead to an inconsistent approach to handling CompletableFuture results across different test cases.

Removing the thenAccept block in Http2ClearTextHessianTest.java might affect the expected output of the test, possibly leading to false positives or negatives in the test results.

Commit 34025eee11c262d09407977be569662acbf78ad0

The patch introduces a small change in the GitHub Actions workflow for a Maven project. The key change is the addition of a newline at the end of the 'maven.yml' file.

Summary of changes:

1 file changed, 1 insertion(+)

Inserted a newline at the end of '.github/workflows/maven.yml'

Potential problems:

No real functionality or improvement is added to the codebase.

The commit message is not clear, which may cause confusion during code review or when browsing the commit history.

Commit 255393c22c7bc5751a07668b3c60d290c1ff5550

This patch makes a change in a single test file (Http2ClearTextHessianTest.java) within the com.alipay.sofa.rpc.transport.http package. The patch removes one line of code in the testHessian() method:

LOGGER.error("CompletableFuture error: {}", e);

Key Changes:

Removal of a log statement used to log errors related to CompletableFuture handling.

Potential problems:

Depending on the purpose of the log statement, removing it might make it harder to debug CompletableFuture errors in this test method in the future. However, since the error is still printed using e.printStackTrace();, debugging might still be possible, but it may not appear in the standard log output.

Commit 4452ae12cb954be4cbe1b78d661d539abdefaef1

Summary of Key Changes:

The patch adds a new Logger class import in the FutureTest.java file.

A new private static final Logger object named LOGGER is created using the LoggerFactory.getLogger(ActivelyDestroyTest.class) method.

Potential Problems:

The LoggerFactory import and logger object creation seem to be correctly implemented. However, there are no actual logger usage or logging calls in the pull request, making it unclear how it would enhance the code.

The patch title "Compatible with CompleteFuture asynchronous programming" does not match the content of this specific patch. The patch only adds a logger and does not seem to make any changes related to CompletableFuture or asynchronous programming. This can cause confusion when reviewing patches or tracking changes in the version control system.

If possible, the patch author should provide more context or an explanation on how this specific change relates to CompletableFuture and how the logger will be used in the test class. The subsequent patches should also be reviewed to ensure they do not introduce any issues in the project.

Commit 945693292b6aaf20d793f162cc96dd84da9a2cbe

This patch introduces a change in the "FutureTest.java" file of the "test-integration" package. The key change made in this patch is updating the class used for instantiating the LOGGER field.

Key Changes:

The LOGGER field instantiation has been updated from LoggerFactory.getLogger(ActivelyDestroyTest.class) to LoggerFactory.getLogger(FutureTest.class).

Potential Problems:

This change seems minimal and not likely to cause any potential issues. The patch correctly assigns the LOGGER to the correct class (FutureTest), which should have been done initially. This change ensures that the logging will be done under the appropriate context.

Commit 63478bc5d617a3e4258d46f4c9b2e255e5f81521

This patch introduces changes in two test files in a GitHub project. The key changes are:

Updating a log message in the FutureTest class.

Adding a new Http2ClearTextHessianTest class with the following changes:

Importing two new classes: Logger and LoggerFactory

Adding a private static Logger instance

Implementing a testHessian() method that incorporates the following changes:

Using a CompletableFuture callback with thenAccept() to log the result inside the testHessian() method

The major concern with this patch is the code inside the Http2ClearTextHessianTest class. Although it adds functionality to test Hessian support over HTTP/2, the test method implementation is not very straightforward and could be improved for better readability and understanding.

There might also be potential issues with exception handling, as the callback for the CompletableFuture is not handling any exceptions, which could lead to unhandled exceptions being thrown during the execution of the test.

Commit 57a1ecc17b3353979e96aa04779eb8079b5c79c5

Summary of key changes:

The AbstractResponseFuture class now extends CompletableFuture<V> and implements ResponseFuture<V> instead of directly extending ResponseFuture<V>.

The ResponseFuture class is changed from an abstract class to an interface and now extends Future<V> and CompletionStage<V>.

Potential problems:

The change in inheritance may cause issues in existing code where AbstractResponseFuture was expected to extend ResponseFuture directly.

The change of ResponseFuture to an interface could cause issues if any existing code was expecting it to be an abstract class, requiring updates to classes that implement this interface.

Since the ResponseFuture now implements Future<V> and CompletionStage<V>, it may introduce incompatibility with other libraries and frameworks expecting it to be a CompletableFuture<V> or other specific implementation.

Commit db141302aa15aba55a310a93306705f4de5ee144

The following is a summary of the key changes and findings:

Two test files have been updated: RpcInternalContextTest.java and InvocationStatDimensionStatTest.java.

A new private class MyResponseFuture, which extends AbstractResponseFuture, has been introduced in both test files.

The implementation of the test cases in both files has been refactored, replacing the anonymous ResponseFuture classes with instances of the new MyResponseFuture class.

Potential problems:

While this change does not introduce any new logic, it is important to ensure that the new MyResponseFuture class has the correct implementation to avoid impacting the test cases' validity.

The MyResponseFuture class seems to have several methods returning null, which might not be the best practice and can lead to NullPointerException in certain scenarios. Consider following the guidelines for handling nulls or providing a more specific implementation for these methods.

The commit message and the Pull Request title do not provide enough context and clarity on the purpose of this change. Consider providing a more descriptive and informative commit message and Pull Request title to help other developers understand the changes.

In conclusion, the changes mostly consist of refactoring the test cases by introducing a new MyResponseFuture class, which extends AbstractResponseFuture. There are no major changes to the overall logic, but it is essential to ensure that the implementations do not introduce unintended behavior.

Commit b85bb7cc694bd68cf8b192707f778b071d0eb2bb

The key change in this GitHub patch is modifying the HystrixResponseFuture class to extend CompletableFuture, rather than implementing the ResponseFuture interface.

The changes can be summarized as follows:

The HystrixResponseFuture class now extends CompletableFuture instead of ResponseFuture.

The patch removes the java.util.concurrent.* import lines for ExecutionException, Future, TimeUnit, and TimeoutException.

Potential problems include:

Compatibility issue: Changing the HystrixResponseFuture to extend CompletableFuture could impact other parts of the codebase that rely on or expect HystrixResponseFuture to implement the ResponseFuture interface.

Unnecessary import removals: Removing imports for ExecutionException, Future, TimeUnit, and TimeoutException could lead to compilation issues if there are usages of these classes within HystrixResponseFuture that were not removed in this patch.

Commit 9c793f55aa44e984624f16440e91973ab677ad6b

This patch is a part of a larger Pull Request focused on making the software compatible with CompleteFuture asynchronous programming.

Key Changes:
The patch modifies the InvocationStatDimensionStatTest.java file within the "fault-tolerance" module.
It adds 6 new lines of code to implement the get(long timeout, TimeUnit unit) method for a class, which is most likely a HystrixResponseFuture implementation.
@Override
public String get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException {
   return null;
}
Potential Problems:

The newly added get(long timeout, TimeUnit unit) method returns null, which may not be the correct or intended behavior. A proper implementation should be provided to avoid null pointer exceptions or unexpected behavior when using this method with CompletableFuture.async.

Commit 6800b15dda1419b963a237a3775ab3acd6391d38

This patch is for a test file in the fault-tolerance module of a software project. The key changes include:

Fixing the code formatting for the get(long timeout, TimeUnit unit) method declaration by moving the TimeoutException to the previous line.

Modifying the prepareInvokeContext() method by slightly changing the instantiation of MyResponseFuture and other indents.

It seems to be a simple formatting fix with no major logical changes. However, it is essential to ensure that the indentation changes do not cause any issues or unintended consequences in the code execution. Also, the commit message is not very clear and informative, but it might not be a significant concern if the patch is part of a series of related changes.

I have a good suggestion: this code review looks like a direct verification of submission records. I think it would be better to compare the final code

For example, Commit 7badd318d2edc53ded7ed851f6cb9f0e945a1f4b，The modifications have already been rolled back when submitting later

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

Overall Summary:

The patch aims to make the project compatible with CompletableFuture asynchronous programming in Java 1.8. It brings a series of changes across multiple classes such as AbstractResponseFuture, ResponseFuture and HystrixResponseFuture. Key changes include modifications in method signatures, class hierarchies, and improvements in type safety. The patch also brings updates to the GitHub Actions workflow and addresses some formatting changes in AbstractResponseFuture.java.

Potential issues:

The changes in class hierarchies and method return types might cause issues in other parts of the project that depend on AbstractResponseFuture or ResponseFuture.
Compatibility with Java versions below 1.8 might be compromised due to the use of CompletableFuture.
There are also changes in the GitHub Actions workflow, removing the release job and Maven authentication secrets, which may cause issues if they were required for the build process.
Several formatting changes have been made in the patch, but their impact on functionality is minimal. More context should be provided regarding the need for these changes and if they adhere to specific coding style guidelines.

Most important findings:

The patch improves compatibility with Java 1.8's CompletableFuture and enhances type safety in some classes.
Developers should ensure that all parts of the code interacting with the modified classes are properly updated to avoid issues.
The patch also refines the GitHub Actions workflow, adding new steps such as running tests and integrating Codecov, but the removal of the release job and Maven authentication secrets might require further consideration.

To ensure a smooth integration of these changes, it is crucial to review and update any dependent code, test the project thoroughly, and have a clear understanding of the rationale behind workflow updates and formatting changes.

Details

Commit 19cb9820d9defa612bb0c84ae82a7cc5b34ea3e6

This patch updates the asynchronous programming compatibility of a Java project, making it work with Java 1.8's CompletableFuture. The key changes include:

The AbstractResponseFuture class is modified to extend ResponseFuture class (previously, it used to implement the ResponseFuture interface).
The ResponseFuture interface is changed to an abstract class, which now extends CompletableFuture.
Both addListeners and addListener methods in ResponseFuture class are marked as abstract and their return type is changed to ResponseFuture instead of the previous void.

Potential problems:

The change in class hierarchy may cause issues in other parts of the project that rely on AbstractResponseFuture or ResponseFuture.
Changing the return type of addListeners and addListener methods in the ResponseFuture class to ResponseFuture instead of void might introduce issues in the existing code that calls these methods and expects a void return type.
Since CompletableFuture is used here, compatibility with Java versions below 1.8 could be compromised.

Commit f5eb764ae3a2588d9c311c0a3b604cbb01270d8e

Key Changes:

The patch updates the AbstractResponseFuture.java file with changes in variable declarations and spacing.
It also changes a looping condition from a blank line for (;;) {} to for (; ;) {}.
Additionally, the patch removes an unused import statement from the ResponseFuture.java file for java.util.concurrent.Future.

Potential Problems:

The spacing changes in variable declarations seem to be minor cosmetic changes or may be related to specific code style guidelines, but do not affect the functionality of the code.
Removing the unused import statement from ResponseFuture.java is a good practice to improve code readability and maintainability, but it also does not affect the code's functionality.

Based on these observations, there are no critical problems or significant changes affecting the code's functionality in the provided patch.

Commit 682ac4005431eadc4ad1fc8f435800b375b8d590

This patch contains changes in two files, making the code compatible with CompletableFuture asynchronous programming introduced in Java 1.8:

ResponseFuture.java: The addListeners and addListener methods now return a generic ResponseFuture<V> instead of the non-generic ResponseFuture. This makes the class more type-safe for working with CompletableFuture.
HystrixResponseFuture.java: The HystrixResponseFuture class is changed to extend ResponseFuture instead of implementing it, likely to provide specific behavior based on ResponseFuture.

While these changes improve type safety and compatibility with CompletableFuture, there could be potential issues if other parts of the code that work with these classes are not updated accordingly. Developers should ensure that all usages are checked and updated if necessary.

Commit 6fe8c4a8429cf878b44c0343a4793da022d0c432

Summary of key changes:

The method signatures of addListeners and addListener have been modified in the ResponseFuture class.
- The return type of both methods have been changed from ResponseFuture<V> to ResponseFuture.

Potential problems:

This change might cause issues in other classes that depend on the previous method signatures having the return type ResponseFuture<V>. They might need to be updated as well to avoid compilation or runtime errors.
Since the patch only shows changes for one file, it's impossible to determine if other parts of the codebase that might be affected by this change. If other classes rely on using the generic type parameters, they might face issues after this change.
The commit message and title seem to indicate the intent is to make the code compatible with Java 1.8's CompletableFuture. However, CompletableFuture is not mentioned anywhere in the provided code changes. The patch may not provide enough context to understand if the intended goal is achieved with these changes.

Commit 964db3c2fcc11abacb8e40bc8adbebf4add19629

The patch makes a change in a single file: AbstractResponseFuture.java. The title of the patch indicates that the change is intended to make the code compatible with Java 1.8's CompletableFuture asynchronous programming.

Key Changes:

The only change in the patch is the addition of a space in the class declaration line of AbstractResponseFuture.

Potential Problems:

While the patch description indicates that this change is associated with making the code compatible with Java 1.8 CompletableFuture, the actual change made does not seem to relate to this purpose.
The patch does not seem to introduce any actual improvements to the code. The file remains the same, except for an additional space that does not impact functionality. Therefore, this patch seems unnecessary and does not contribute any value to the project.

Overall, this patch does not introduce any meaningful changes to the codebase and does not address the stated goal of improving compatibility with Java 1.8 CompletableFuture.

Commit 7badd318d2edc53ded7ed851f6cb9f0e945a1f4b

The patch updates the GitHub Actions workflow (.github/workflows/maven.yml) for a Java project built with Maven. Here's a summary of the key changes:

The workflow is now triggered only by manual workflow_dispatch events, removing triggers for push and pull_request events on the master branch.
The actions/checkout step now uses version v3 instead of v2.
A new release job has been added, which depends on the successful completion of the build job. It is configured to run on ubuntu-latest and deploys the project with Maven in batch mode, skipping tests, and using the release profile. The job also sets up the JDK 8, Maven caching, and GPG private keys for signing artifacts.
The environment variables for Maven username, password, and GPG passphrase are fetched from GitHub secrets.

Potential problems:

The workflow is no longer triggered by push and pull_request events, which might not be ideal if the goal is to automatically build and test the project on every change. If that's the case, the removed triggers should be added back.
The release job deploys the project on every manual workflow_dispatch. Ideally, the deployment should only be triggered when a new version or release is created. To handle this, you may want to add a condition to only run the release job when pushing a new tag or using another appropriate trigger for deployment purposes.

Commit e1d0a9b1d4c58ffd3af39ca22f39b616e6bcf2db

This GitHub patch introduces changes to the AbstractResponseFuture.java file in the SOFARPC framework.

Key changes:

Formatting adjustments have been made to the declarations of 'result', 'cause', 'timeout', 'genTime', 'sentTime', and 'doneTime' instance variables.
A try-catch block has been slightly modified in the await0() method.

Potential Problems:

There don't seem to be any functional changes in the patch. All modifications appear to be related to code formatting and style.
While it may not introduce any issues, the fact that only formatting modifications are introduced makes it difficult to assess the actual implications of the changes.

Commit b7944a22fdfb498cda13a1f4631976292a46a774

This patch updates the .github/workflows/maven.yml file, making changes to the build workflow. The key changes are as follows:

The updated workflow triggers on push events and pull requests on the master branch, as well as manual workflow dispatch events.
The steps within the "build" job have been modified; now it sets up JDK 8 and runs the build and format check using Maven.
The "release" job has been removed, and its functionality is not covered in the updated workflow.
A new step has been added to run tests using Maven.
Codecov integration has been added using the codecov-action.

Potential problems:

The removal of the "release" job may be an issue if releasing the software through the workflow was a requirement.
The secrets for Maven authentication (MAVEN_USERNAME, MAVEN_PASSWORD, and MAVEN_GPG_PASSPHRASE) have been removed, which may cause issues if they were required for the build process.

Commit 87311b356e25cfa81f5f53f880908a9a110a689f

This patch introduces changes to the AbstractResponseFuture.java file of a software project.

Key Changes:

The patch modifies the class declaration by removing an extra space. The updated line is now: public abstract class AbstractResponseFuture<V> extends ResponseFuture<V> {
The await method's signature has been updated to fix the alignment of throws InterruptedException. The updated signature is: protected boolean await(long timeout, TimeUnit unit) throws InterruptedException {
The last line of the file had an unnecessary newline character which was removed.

The most important findings in this patch are the changes in method signatures and formatting in the class declaration. There are no potential problems introduced by this patch as it only involves formatting updates and does not change any functionality.

Commit 8c06b4e158b318f477d74a7a78eecbaf0e925574

Summary of key changes: In the patch, the developer has made a change to the await method signature in the AbstractResponseFuture.java file. There is a change in the indentation of the "throws InterruptedException" clause. The method signature before the change was:

protected boolean await(long timeout, TimeUnit unit) throws InterruptedException {

After the change:

protected boolean await(long timeout, TimeUnit unit)
        throws InterruptedException {

Potential problems:

The only change in this patch is related to formatting. There are no functional or logic changes made to the code.
No additional information or context is provided in the message or comments to explain the need for this formatting change.

Considering that the only change is related to formatting, reviewing this Pull Request is relatively straightforward. The proposed formatting change does not introduce any functional issues or conflicts with existing code. However, it would be beneficial for the developer to provide more context on the reason for this formatting change, and if it's meant to adhere to a specific coding style or guideline.

Commit aa9a1be5ce4bb03172c61c5e416a3a1b368edf08

The key changes in this patch include:

A new test block has been added to the existing testAll() method in the AsyncChainTest class.
The CompletableFuture result has been chained to an asynchronous callback handler using the thenAccept method.
The result of the CompletableFuture is logged via LOGGER.info.

Potential problems:

The code lacks comments and could be difficult to understand, particularly for developers who may not be familiar with the CompletableFuture concept.
It is not clear how the new test block interacts with the existing testAll() method, potentially making it difficult to understand this section of the code.
The CompletableFuture result handler does not handle any exceptions that could be thrown during its execution. This could result in unhandled exceptions or unexpected behavior in case of errors.

To improve the patch, the following recommendations can be made:

Add comments explaining the CompletableFuture usage and the purpose of the additional test block.
Consider refactoring the testAll() method to separate the different test scenarios and improve readability.
Add proper exception handling in the thenAccept section using exceptionally or handle methods of CompletableFuture.

Commit f8250df2d1088737b1e8975d539f858dec1ad931

Summary: The patch contains a change in a single file, ".github/workflows/maven.yml". The key change made is the replacement of one line of code related to the Codecov GitHub action version being used in the workflow.

Changes:

In ".github/workflows/maven.yml", the previous version of the Codecov GitHub action (codecov/codecov-action@v1) is replaced with the same version reference.

Potential Problems:

The patch title ("Compatible with CompleteFuture asynchronous programming") is not directly related to the code changes.
There is no apparent functional modification, as the same version of the Codecov GitHub Action is specified before and after the change.
The missing newline at the end of the file is generally regarded as a minor issue but may cause issues with some tools and standards.

Commit 0a1b40755a05d02483ee6f9751038001c8498508

This patch makes changes to the AsyncChainTest.java file in the test suite of a Java project.

Key Changes:

The patch comments out the usage of the thenAccept method on a CompletableFuture instance. This is done by replacing three lines (92-94) with three commented-out lines (93-95).

Potential Problems:

Commenting out the usage of the thenAccept method might disable the verification of the asynchronous chain functionality, which requires testing the feature that combines CompletableFuture within the codebase. This could potentially lead to untested scenarios or missed bugs.
Since no alternative testing mechanism has been implemented, this might weaken the test coverage on verifying the compatibility with CompletableFuture asynchronous programming.

Commit 4d13a3e5da1fe2138689ac3ec279266954508b38

The key changes in this patch include:

The patch is modifying a test file AsyncChainTest.java in the project.
It enables a previously commented-out line of code for CompletableFuture by removing comment markers.
This change seems to enable asynchronous callback handling with CompletableFuture in the test method testAll().

Potential problems:

There could be potential issues if the callback is not correctly implemented or if the CompletableFuture does not function as intended, causing inconsistencies with the test behavior.
Since the previous configuration had this code commented out, there could be underlying reasons why it was disabled in the first place, which may re-introduce possible issues or bugs.

It is essential to investigate the reason behind the initial setup and understand potential risks before merging the pull request.

Commit 34c4c092a5d1b606ba78f09b055d16a3c03255f6

This patch makes changes to a single Java test file, AsyncChainTest.java, within the "Compatible with CompleteFuture asynchronous programming" pull request.

Key Changes:

The asynchronous CompletableFuture invocation is now wrapped in a try-catch block to handle potential exceptions.
If an exception occurs during CompletableFuture processing, an error log will be generated.

Potential Problems:

The catch block uses "{}" as a placeholder for the exception object, which might not provide enough information about the exception in the log message.
The patch does not address related tests or other parts of the codebase that might be affected by the changes made in AsyncChainTest.java.

Commit adf85aab61526791ac878deb769fd60158fc2a7e

This patch makes changes to two files, AsyncChainTest.java, and Http2ClearTextHessianTest.java.

In AsyncChainTest.java, 8 lines of code related to handling CompletableFuture within the testAll() method have been removed. This code block was handling an asynchronous call by using the thenAccept() method on the CompletableFuture object.

In Http2ClearTextHessianTest.java, 4 lines of code have been added within the testHessian() method. It introduces the thenAccept() method on the CompletableFuture object in this method, which logs the result of the CompletableFuture.

Key changes:

Removed CompletableFuture handling in AsyncChainTest.java.
Added CompletableFuture handling in Http2ClearTextHessianTest.java.

Potential Problems:

The removal of the CompletableFuture handling in AsyncChainTest.java may cause incorrect or incomplete test behavior since the asynchronous callback handling has been removed. This could make the test results unreliable or even cause the test case to fail.
The newly added CompletableFuture handling in Http2ClearTextHessianTest.java may affect the maintainability, readability, and ease of understanding of the code, especially if the handling had been removed from AsyncChainTest.java in favor of a more robust or modular approach.

Commit f7cda08c6648bd25f502eed0e54786e3475b258e

The pull request "Compatible with CompleteFuture asynchronous programming" includes changes to two files, with the primary goal of ensuring that both files end with a newline character. This improves compatibility and follows best practices.

Key Changes:

Two files have been modified:
- .github/workflows/maven.yml
- core/api/src/main/java/com/alipay/sofa/rpc/message/AbstractResponseFuture.java
Both files' changes involve inserting a newline character at the end of the file.

While there are no major issues in this pull request, it is worth noting that the changes made are quite minimal and do not address any functionality updates concerning CompletableFuture asynchronous programming indicated by the title.

Commit 8efc6246942aa226d4629463f8bc3760f00cedb7

The key changes in this patch are:

Added a new test case in FutureTest.java that tests the compatibility with CompleteFuture asynchronous programming. It does the following:
- Invokes the helloService.sayHello() method with parameters "xxx" and 22.
- Asserts that the return value is null after the first invocation.
- Adds a callback function thenAccept() to the future returned by RpcInvokeContext.getContext().getFuture() that logs the result when it's available.
- Waits for 1.5 seconds to ensure the RPC timeout has been triggered.
- Retrieves the future using SofaResponseFuture.getFuture().
- Gets the result from the future and assigns it to 'ret'.
Removed a redundant piece of code from Http2ClearTextHessianTest.java that was setting a callback function thenAccept() on the future returned by RpcInvokeContext.getContext().getFuture(). The response is already being asserted in the try block later.

Potential problems:

The change (removal of redundant code) in Http2ClearTextHessianTest.java should not have any impact as it was redundant and did not affect the test.
The new test case in FutureTest.java could potentially cause flakiness in the test suite if the RPC timeout or the asynchronous operation takes longer than expected. However, the test itself doesn't have any identified issues.

Commit 34025eee11c262d09407977be569662acbf78ad0

This GitHub patch updates the maven.yml file, which is part of the GitHub Actions configuration for a Maven-based project. The key change in this Pull Request is adding an additional line at the end of the file.

Key changes:

Added a new line at the end of the maven.yml file.

Potential problems:

The title of the Pull Request does not provide enough information to understand the changes being made. A more descriptive title would help other developers understand and review the changes.
The commit message is not clear and has strange characters (encoded in UTF-8) due to encoding errors, making it less informative and difficult to understand.
The actual changes (just adding a newline) seem unrelated to the title of the Pull Request, which implies that it should have something to do with CompletableFuture asynchronous programming.

Commit 255393c22c7bc5751a07668b3c60d290c1ff5550

This patch makes changes to the file Http2ClearTextHessianTest.java in the test suite for the project. Specifically, it removes a single line of code that logs an error message when there is an exception thrown in the testHessian method.

Key Changes:

Removed the LOGGER.error("CompletableFuture error: {}", e); line in the testHessian method, which was responsible for logging the error when an exception occurs.

Potential Problems:

No significant problems are introduced in the code by this patch, as it only removes an error logging line.
However, eliminating this error log might make it harder to debug any issues with the testHessian method in the future, as the relevant information won't be logged. If this log message is crucial for debugging or monitoring purposes, it should be retained.

Commit 4452ae12cb954be4cbe1b78d661d539abdefaef1

The key changes in this pull request are:

The addition of a new Logger class with an import statement:

import com.alipay.sofa.rpc.log.Logger;
import com.alipay.sofa.rpc.log.LoggerFactory;

The addition of a LOGGER instance in the FutureTest class:

private static final Logger LOGGER = LoggerFactory.getLogger(ActivelyDestroyTest.class);

Potential problems:

The LOGGER instance is never used in the code. This suggests that the LOGGER might not be necessary, or the changes to implement logging were not included in the pull request.
There is also an import statement for TripleHessianInvokeTest which, as far as the provided patch is concerned, is not being used. This might be an unnecessary import.

It would be helpful to have more context of the overall changes in the pull request or a description of what the developer intended to achieve with these changes.

Commit 945693292b6aaf20d793f162cc96dd84da9a2cbe

This patch is a part of a larger pull request regarding "Compatible with CompleteFuture asynchronous programming". The changes made in this patch include:

In the FutureTest.java file, there is a modification to the Logger initialization:
- The old Logger initialization used the ActivelyDestroyTest class to instantiate the logger: LoggerFactory.getLogger(ActivelyDestroyTest.class).
- The updated Logger initialization uses the FutureTest class instead: LoggerFactory.getLogger(FutureTest.class).

Key findings:

The change in the patch seems appropriate as it updated the logger to be initialized with the correct class, improving maintainability and readability.
On its own, this patch does not seem to have any problems.

It is important to note that this is only a small part of the complete pull request, and the overall compatibility with CompleteFuture asynchronous programming should be evaluated using the entire set of patches.

Commit 63478bc5d617a3e4258d46f4c9b2e255e5f81521

This patch includes changes in two test files: FutureTest.java and Http2ClearTextHessianTest.java.

Key changes:

In FutureTest.java, a log message has been updated from "RpcInvokeContext CompletableFuture result" to "FutureTest RpcInvokeContext CompletableFuture result".
In Http2ClearTextHessianTest.java:
- Added new Logger instance for this class.
- A CompletableFuture's thenAccept method is used to log the response after the asynchronous completion.

Potential problems:

No major issues. However, in Http2ClearTextHessianTest.java, make sure that necessary import statements have been added for the Logger and LoggerFactory classes if they weren't present in the codebase before this patch.
For better readability and understanding, consider adding comments to explain the purpose of the newly added CompletableFuture's thenAccept method block in Http2ClearTextHessianTest.

Commit 57a1ecc17b3353979e96aa04779eb8079b5c79c5

Summary of key changes:

In the AbstractResponseFuture class, the inheritance of ResponseFuture has been replaced with CompletableFuture. This means that AbstractResponseFuture now extends CompletableFuture and implements the ResponseFuture interface.
In the ResponseFuture class, the original ResponseFuture abstract class has been converted to an interface. It now extends two interfaces: Future and CompletionStage.
The public abstract methods in the ResponseFuture interface now do not have the "public" and "abstract" keywords, as they are automatically public and abstract in an interface.

Potential problems:

Changing inheritance and converting classes to interfaces may lead to compatibility issues, especially if other classes rely on this code. Existing projects may need to be updated to accommodate these changes.
By extending CompletableFuture directly in the AbstractResponseFuture class, it's now more tightly coupled to the CompletableFuture implementation. This may have implications for future changes or additional functionality.
The code no longer makes use of the CompletableFuture class within the ResponseFuture interface, instead depending on CompletionStage. This change might impact the existing behavior or functionality.

Commit db141302aa15aba55a310a93306705f4de5ee144

This patch modifies two test classes: RpcInternalContextTest and InvocationStatDimensionStatTest. Here is a summary of the key changes:

A new private inner class MyResponseFuture has been added in both test classes, extending AbstractResponseFuture<String>. This class defines several methods, including addListener(SofaResponseCallback sofaResponseCallback), addListeners(List<SofaResponseCallback> sofaResponseCallbacks), and overrides some other methods such as isDone(), isCancelled(), etc.
In RpcInternalContextTest, the testClear() method has been updated to use MyResponseFuture instead of an anonymous implementation of ResponseFuture<String>.
In InvocationStatDimensionStatTest, the prepareInvokeContext() method has been updated to use MyResponseFuture as well.

Potential problems:

The methods addListener(SofaResponseCallback sofaResponseCallback) and addListeners(List<SofaResponseCallback> sofaResponseCallbacks) in MyResponseFuture are returning null. If this is intentional, it should be clearly documented, otherwise, there may be unexpected behavior or exceptions when using these methods. Optionally, consider using the keyword void if these methods are not meant to return anything.
There are no new test cases added specifically for the new MyResponseFuture class. If this functionality is critical to the application, it should have corresponding test cases that cover its usage and expected behavior.

Commit b85bb7cc694bd68cf8b192707f778b071d0eb2bb

This patch modifies the HystrixResponseFuture class in the fault-hystrix module. The key changes are:

The class HystrixResponseFuture is changed to extend CompletableFuture instead of ResponseFuture.
An unused import is removed, and five other imports are consolidated into a single import for java.util.concurrent.*.

By extending CompletableFuture, the HystrixResponseFuture class becomes compatible with the CompletableFuture asynchronous programming model. However, there might be potential problems:

Changing the inheritance from ResponseFuture to CompletableFuture might cause issues if any methods in ResponseFuture class are used in other parts of the code.
The logic inside HystrixResponseFuture class for handling the asynchronous execution is not shown. It is important to make sure the code is updated to properly use the CompletableFuture APIs instead of the old Future APIs.

It is recommended to investigate the potential issues mentioned above and ensure proper testing is conducted with the updated HystrixResponseFuture class.

Commit 9c793f55aa44e984624f16440e91973ab677ad6b

This GitHub patch contains changes made to the test file InvocationStatDimensionStatTest.java. The key changes include:

Added implementation of get(long timeout, TimeUnit unit) method, which is part of the java.util.concurrent.Future interface. This method is used to wait for a result for a specified time and checks if the value is available, otherwise throws a TimeoutException.

Potential problems:

The implementation of the newly added get(long timeout, TimeUnit unit) method simply returns null, without actually implementing any functionality. This might not be the desired behavior and could potentially lead to errors or unexpected application behavior during runtime.
No test cases are added for the new method implementation. To ensure that the implementation works as expected, it's necessary to add test cases covering different scenarios.

Commit 6800b15dda1419b963a237a3775ab3acd6391d38

This patch modifies a single Java file InvocationStatDimensionStatTest.java.

Key changes:

In the get() method of the anonymous subclass of HystrixResponseFuture, the throws clause indentation is adjusted.
In the onAppResponse() method of the anonymous subclass of InvokeCallback, a code formatting change is made: the opening brace of a new MyResponseFuture instance is moved to the new line.

Potential problems:

There are no substantial changes to the functionality of the code, only formatting adjustments.
Since it's a formatting change, it might not have any significant impact on the code's functionality, but it's crucial to maintain a consistent code style across the project.

Commit 88602afd65ea3a5e8a945fa7b6af73d4e2c47e68

Summary of key changes:

The method addListeners in ResponseFuture interface has been updated. The return type has been adjusted from ResponseFuture to ResponseFuture<V>.
The method addListener in ResponseFuture interface has also been updated. The return type has been adjusted from ResponseFuture to ResponseFuture<V>.

Potential problems:

This change will affect all the implementations of ResponseFuture interface. If there are any existing implementations of this interface, their method signatures must be updated accordingly to match the updated interface.
This change can potentially cause backward compatibility issues for clients consuming this library if they are explicitly relying on the previous method return types.
Further testing should be conducted to ensure that after these changes, the asynchronous programming-related features are working as expected.

@OrezzerO @EvenLjj This merger request is a bit long, do you have time to help review it and merge branch?

@OrezzerO @EvenLjj This merger request is a bit long, do you have time to help review it and merge branch?

OK, but this PR will also affect the internal version, and we will review in the next iteration and merge after passing the internal regression test.

@OrezzerO @EvenLjj This merger request is a bit long, do you have time to help review it and merge branch?

OK, but this PR will also affect the internal version, and we will review in the next iteration and merge after passing the internal regression test.

OK, i get it

 @Test
    public void thenApply() throws InterruptedException {
        AtomicBoolean applied = new AtomicBoolean(false);
        MyFuture myFuture = new MyFuture(1000);
        Consumer<? super String> consumer = (String str) -> {
            System.out.println(str);
            applied.compareAndSet(false, true);
        };
        myFuture.thenAccept(consumer);

        myFuture.setSuccess("done");

        Thread.sleep(1000);

        Assert.assertTrue(applied.get());

    }

    @Test
    public void thenApply_correctVersion() throws InterruptedException {
        AtomicBoolean applied = new AtomicBoolean(false);
        CountDownLatch countDownLatch = new CountDownLatch(1);
        CompletableFuture<String> future = CompletableFuture.supplyAsync(() -> {
            // 模拟耗时操作
            try {
                countDownLatch.await();
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
            return "Hello, World!";
        });

        Consumer<? super String> consumer = (String str) -> {
            System.out.println(str);
            applied.compareAndSet(false, true);
        };
        future.thenAccept(consumer);

        countDownLatch.countDown();

        Thread.sleep(1000);

        Assert.assertTrue(applied.get());

    }

I write two test case . The first one is aim to test AbstractResponseFuture, and the second one shows how CompletableFuture should work. Please make sure the first case can pass.

@Test
    public void thenApply() throws InterruptedException {
        AtomicBoolean applied = new AtomicBoolean(false);
        MyFuture myFuture = new MyFuture(1000);
        Consumer<? super String> consumer = (String str) -> {
            System.out.println(str);
            applied.compareAndSet(false, true);
        };
        myFuture.thenAccept(consumer);

        myFuture.setSuccess("done");

        boolean isDone = myFuture.complete("done");

        Thread.sleep(1000);

        Assert.assertTrue(applied.get());

    }

I found that future does not perform an operation, it needs to call complete method to modify the state @OrezzerO

Yes, you are right. So there is a compatibility issues here. We need to make sure that when future task is complete, then it trigger 'then apply' task. complete method is provided by CompletableFuture , it will not invoked by sofarpc code. We need to invoke method such as complete in sofarpc some where.

I think this pr is ok. @EvenLjj Please check compatibility with internal version .

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@EvenLjj Buddy, this pr is a bit long and will be marked as expired. Evaluate whether the merger will affect the existing code, thanks

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sofastack / sofa-rpc