You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@eventmesh.apache.org by "pandaapo (via GitHub)" <gi...@apache.org> on 2023/04/25 12:19:16 UTC

[GitHub] [eventmesh] pandaapo opened a new pull request, #3819: [ISSUE #3815]DelayRetryable and RetryContext can be refactored.

pandaapo opened a new pull request, #3819:
URL: https://github.com/apache/eventmesh/pull/3819

   Fixes #3815.
   
   ### Motivation
   
   *See "Enhancement Request" in the issue.*
   
   
   
   ### Modifications
   
   *See "Describe the solution you'd like" in the issue*
   
   
   
   ### Documentation
   
   - Does this pull request introduce a new feature?  no
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org

[GitHub] [eventmesh] pandaapo commented on a diff in pull request #3819: [ISSUE #3815]DelayRetryable and RetryContext can be refactored.

Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #3819:
URL: https://github.com/apache/eventmesh/pull/3819#discussion_r1186184235


##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/http/producer/SendMessageContext.java:
##########
@@ -123,13 +123,13 @@ public String toString() {
     }
 
     @Override
-    public boolean retry() throws Exception {
+    public void retry() throws Exception {
         if (eventMeshProducer == null) {
-            return false;
+            log.error("Exception happends during retry. EventMeshProduceer is null.");

Review Comment:
   I was merging 3 `RetryContext`s in module http, grpc and tcp to one.
   ![1683297249354](https://user-images.githubusercontent.com/35672972/236488064-32bcba14-2deb-403e-9de1-2ea9ed9bd22b.png)
   3 `RetryContext`s have almost same structure except a little difference. This `retry()` is that difference. It returns boolean in module http, grpc, and void in module tcp. Then I checked all calls of `retry()` that returns boolean. I found the return value of boolean has no practical use in all calls. So I chose to modify it to `void` to keep with `retry()` in module tcp.
   
   If the return value of boolean type is indeed useful, may I modify the void to boolean in module tcp?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org

[GitHub] [eventmesh] xwm1992 merged pull request #3819: [ISSUE #3815]DelayRetryable and RetryContext can be refactored.

Posted by "xwm1992 (via GitHub)" <gi...@apache.org>.
xwm1992 merged PR #3819:
URL: https://github.com/apache/eventmesh/pull/3819


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org

[GitHub] [eventmesh] codecov[bot] commented on pull request #3819: [ISSUE #3815]DelayRetryable and RetryContext can be refactored.

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3819:
URL: https://github.com/apache/eventmesh/pull/3819#issuecomment-1522735940

   ## [Codecov](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3819](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0369d61) into [master](https://codecov.io/gh/apache/eventmesh/commit/a2300cd1edc6833a63f4a2cc4534b260f4ed7bcd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2300cd) will **decrease** coverage by `0.01%`.
   > The diff coverage is `0.00%`.
   
   > :exclamation: Current head 0369d61 differs from pull request most recent head 0fb7ab2. Consider uploading reports for the commit 0fb7ab2 to get more accurate results
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3819      +/-   ##
   ============================================
   - Coverage     13.78%   13.78%   -0.01%     
     Complexity     1290     1290              
   ============================================
     Files           571      569       -2     
     Lines         29137    29112      -25     
     Branches       2849     2833      -16     
   ============================================
   - Hits           4017     4013       -4     
   + Misses        24747    24727      -20     
   + Partials        373      372       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../eventmesh/runtime/core/protocol/RetryContext.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvUmV0cnlDb250ZXh0LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...ore/protocol/grpc/producer/SendMessageContext.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvZ3JwYy9wcm9kdWNlci9TZW5kTWVzc2FnZUNvbnRleHQuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...e/core/protocol/grpc/push/AbstractPushRequest.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvZ3JwYy9wdXNoL0Fic3RyYWN0UHVzaFJlcXVlc3QuamF2YQ==) | `0.00% <ø> (ø)` | |
   | [.../runtime/core/protocol/grpc/retry/GrpcRetryer.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvZ3JwYy9yZXRyeS9HcnBjUmV0cnllci5qYXZh) | `0.00% <ø> (ø)` | |
   | [...ore/protocol/http/producer/SendMessageContext.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvaHR0cC9wcm9kdWNlci9TZW5kTWVzc2FnZUNvbnRleHQuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...re/protocol/http/push/AbstractHTTPPushRequest.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvaHR0cC9wdXNoL0Fic3RyYWN0SFRUUFB1c2hSZXF1ZXN0LmphdmE=) | `0.00% <ø> (ø)` | |
   | [.../core/protocol/http/push/AsyncHTTPPushRequest.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvaHR0cC9wdXNoL0FzeW5jSFRUUFB1c2hSZXF1ZXN0LmphdmE=) | `0.00% <ø> (ø)` | |
   | [.../runtime/core/protocol/http/retry/HttpRetryer.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvaHR0cC9yZXRyeS9IdHRwUmV0cnllci5qYXZh) | `0.00% <ø> (ø)` | |
   | [.../tcp/client/session/push/DownStreamMsgContext.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvdGNwL2NsaWVudC9zZXNzaW9uL3B1c2gvRG93blN0cmVhbU1zZ0NvbnRleHQuamF2YQ==) | `0.00% <ø> (ø)` | |
   | [.../tcp/client/session/retry/EventMeshTcpRetryer.java](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXZlbnRtZXNoLXJ1bnRpbWUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC9ydW50aW1lL2NvcmUvcHJvdG9jb2wvdGNwL2NsaWVudC9zZXNzaW9uL3JldHJ5L0V2ZW50TWVzaFRjcFJldHJ5ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | ... and [1 more](https://codecov.io/gh/apache/eventmesh/pull/3819?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ... and [6 files with indirect coverage changes](https://codecov.io/gh/apache/eventmesh/pull/3819/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org

[GitHub] [eventmesh] pandaapo commented on a diff in pull request #3819: [ISSUE #3815]DelayRetryable and RetryContext can be refactored.

Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #3819:
URL: https://github.com/apache/eventmesh/pull/3819#discussion_r1186602748


##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/http/producer/SendMessageContext.java:
##########
@@ -123,13 +123,13 @@ public String toString() {
     }
 
     @Override
-    public boolean retry() throws Exception {
+    public void retry() throws Exception {
         if (eventMeshProducer == null) {
-            return false;
+            log.error("Exception happends during retry. EventMeshProduceer is null.");

Review Comment:
   Oh! Yes. :joy:



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org

[GitHub] [eventmesh] pandaapo commented on a diff in pull request #3819: [ISSUE #3815]DelayRetryable and RetryContext can be refactored.

Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #3819:
URL: https://github.com/apache/eventmesh/pull/3819#discussion_r1186184235


##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/http/producer/SendMessageContext.java:
##########
@@ -123,13 +123,13 @@ public String toString() {
     }
 
     @Override
-    public boolean retry() throws Exception {
+    public void retry() throws Exception {
         if (eventMeshProducer == null) {
-            return false;
+            log.error("Exception happends during retry. EventMeshProduceer is null.");

Review Comment:
   I was merging 3 `RetryContext`s in module http, grpc and tcp to one.
   ![1683322381305](https://user-images.githubusercontent.com/35672972/236572429-f594b8ca-bca3-4e35-9f1f-15af71a4a107.png)
   3 `RetryContext`s have almost same structure except a little difference. This `retry()` is that difference. It returns boolean in module http, grpc, and void in module tcp. Then I checked all calls of `retry()` that returns boolean. I found the return value of boolean has no practical use in all calls. So I chose to modify it to `void` to keep with `retry()` in module tcp.
   
   If the return value of boolean type is indeed useful, may I modify the void to boolean in module tcp?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org

[GitHub] [eventmesh] xwm1992 commented on a diff in pull request #3819: [ISSUE #3815]DelayRetryable and RetryContext can be refactored.

Posted by "xwm1992 (via GitHub)" <gi...@apache.org>.
xwm1992 commented on code in PR #3819:
URL: https://github.com/apache/eventmesh/pull/3819#discussion_r1185902661


##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/http/producer/SendMessageContext.java:
##########
@@ -123,13 +123,13 @@ public String toString() {
     }
 
     @Override
-    public boolean retry() throws Exception {
+    public void retry() throws Exception {
         if (eventMeshProducer == null) {
-            return false;
+            log.error("Exception happends during retry. EventMeshProduceer is null.");
         }
 
         if (retryTimes > 0) { //retry once
-            return false;
+            log.error("Exception happends during retry. The retryTimes > 0.");

Review Comment:
   same as above



##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/http/producer/SendMessageContext.java:
##########
@@ -123,13 +123,13 @@ public String toString() {
     }
 
     @Override
-    public boolean retry() throws Exception {
+    public void retry() throws Exception {
         if (eventMeshProducer == null) {
-            return false;
+            log.error("Exception happends during retry. EventMeshProduceer is null.");

Review Comment:
   here need add return in if block



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org

[GitHub] [eventmesh] pandaapo commented on a diff in pull request #3819: [ISSUE #3815]DelayRetryable and RetryContext can be refactored.

Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #3819:
URL: https://github.com/apache/eventmesh/pull/3819#discussion_r1186184235


##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/http/producer/SendMessageContext.java:
##########
@@ -123,13 +123,13 @@ public String toString() {
     }
 
     @Override
-    public boolean retry() throws Exception {
+    public void retry() throws Exception {
         if (eventMeshProducer == null) {
-            return false;
+            log.error("Exception happends during retry. EventMeshProduceer is null.");

Review Comment:
   I was merging 3 `RetryContext`s in module http, grpc and tcp to one.
   ![1683322250147](https://user-images.githubusercontent.com/35672972/236572151-21f75011-b51d-4135-8d94-0c5b81ffdafb.png)
   3 `RetryContext`s have almost same structure except a little difference. This `retry()` is that difference. It returns boolean in module http, grpc, and void in module tcp. Then I checked all calls of `retry()` that returns boolean. I found the return value of boolean has no practical use in all calls. So I chose to modify it to `void` to keep with `retry()` in module tcp.
   
   If the return value of boolean type is indeed useful, may I modify the void to boolean in module tcp?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org

[GitHub] [eventmesh] xwm1992 commented on a diff in pull request #3819: [ISSUE #3815]DelayRetryable and RetryContext can be refactored.

Posted by "xwm1992 (via GitHub)" <gi...@apache.org>.
xwm1992 commented on code in PR #3819:
URL: https://github.com/apache/eventmesh/pull/3819#discussion_r1186592647


##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/http/producer/SendMessageContext.java:
##########
@@ -123,13 +123,13 @@ public String toString() {
     }
 
     @Override
-    public boolean retry() throws Exception {
+    public void retry() throws Exception {
         if (eventMeshProducer == null) {
-            return false;
+            log.error("Exception happends during retry. EventMeshProduceer is null.");

Review Comment:
   What I mean is that in your modified logic, the return is missing in the if code block, and return boolean is not required.
   you need add `return` after the `log.error()`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org