You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@servicecomb.apache.org by GitBox <gi...@apache.org> on 2020/12/21 09:50:13 UTC

[GitHub] [servicecomb-java-chassis] liuguangrong opened a new pull request #2154: Edge's 490 exception is thrown and can be used for business processing

liuguangrong opened a new pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154


   Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SCB) filed for the change (usually before you start working on it).  Trivial changes like typos do not require a JIRA issue.  Your pull request should address just this issue, without pulling in other changes.
    - [ ] Each commit in the pull request should have a meaningful subject line and body.
    - [ ] Format the pull request title like `[SCB-XXX] Fixes bug in ApproximateQuantiles`, where you replace `SCB-XXX` with the appropriate JIRA issue.
    - [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
    - [ ] Run `mvn clean install -Pit` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   ---
   


----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] liubao68 commented on a change in pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
liubao68 commented on a change in pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#discussion_r547244883



##########
File path: transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/http/RestClientInvocation.java
##########
@@ -116,12 +118,17 @@ public void invoke(Invocation invocation, AsyncResponse asyncResp) throws Except
         filter.beforeSendRequest(invocation, requestEx);
       }
     }
+    List<HttpRestClientInvocationHook> httpRestClientInvocationHooks =
+            SPIServiceUtils.getAllService(HttpRestClientInvocationHook.class);
 
     clientRequest.exceptionHandler(e -> {
       invocation.getTraceIdLogger()
           .error(LOGGER, "Failed to send request, alreadyFailed:{}, local:{}, remote:{}, message={}.",
               alreadyFailed, getLocalAddress(), ipPort.getSocketAddress(),
               ExceptionUtils.getExceptionMessageWithoutTrace(e));
+      httpRestClientInvocationHooks.forEach(httpServerExceptionHandler -> {

Review comment:
       ```
   public class MyHandler implements Handler {
   
     private static final Logger LOGGER = LoggerFactory.getLogger(MyHandler.class);
   
     @Override
     public void handle(Invocation invocation, AsyncResponse asyncResp) throws Exception {
       LOGGER.info("before");
   
       invocation.next(response -> {
            LOGGER.info("after");
       });
     }
   }
   ```
   
   You can find many Handler implementations in servicecomb-java-chassis project. 




----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] Volcannozzz commented on pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
Volcannozzz commented on pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#issuecomment-773099927


   I think the handler example given by @liubao68  cannot meet @liuguangrong 's needs. Because the transportHandler is the last handler in the source code.
   
   ```java
   public class ConsumerHandlerManager extends AbstractHandlerManager {
     @Override
     protected String getName() {
       return "Consumer";
     }
   
     @Override
     protected String getInnerDefaultChainDef() {
       return "simpleLB";
     }
   
     @Override
     protected Handler getLastHandler() {
       return TransportClientHandler.INSTANCE;
     }
   }
   ```
   and in `AbstractHandlerManager.java`
   
   ```java
     private List<Handler> createHandlerChain(String chainDef) {
       List<Class<Handler>> chainClasses = convertToChainClass(chainDef);
   
       List<Handler> handlerList = new ArrayList<>();
       for (Class<Handler> cls : chainClasses) {
         try {
           handlerList.add(cls.newInstance());
         } catch (Exception e) {
           // 在启动阶段直接抛异常出来
           throw new Error(e);
         }
       }
       handlerList.add(getLastHandler());
       return handlerList;
     }
   ```
   In addition, when the transportHandler calls an exception, it will return the exception information to the caller. Can the exception information be handled by the caller? Hope you provide the calling way and error message stack.


----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] Volcannozzz commented on pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
Volcannozzz commented on pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#issuecomment-773099927


   I think the handler example given by @liubao68  cannot meet @liuguangrong 's needs. Because the transportHandler is the last handler in the source code.
   
   ```java
   public class ConsumerHandlerManager extends AbstractHandlerManager {
     @Override
     protected String getName() {
       return "Consumer";
     }
   
     @Override
     protected String getInnerDefaultChainDef() {
       return "simpleLB";
     }
   
     @Override
     protected Handler getLastHandler() {
       return TransportClientHandler.INSTANCE;
     }
   }
   ```
   and in `AbstractHandlerManager.java`
   
   ```java
     private List<Handler> createHandlerChain(String chainDef) {
       List<Class<Handler>> chainClasses = convertToChainClass(chainDef);
   
       List<Handler> handlerList = new ArrayList<>();
       for (Class<Handler> cls : chainClasses) {
         try {
           handlerList.add(cls.newInstance());
         } catch (Exception e) {
           // 在启动阶段直接抛异常出来
           throw new Error(e);
         }
       }
       handlerList.add(getLastHandler());
       return handlerList;
     }
   ```
   In addition, when the transportHandler calls an exception, it will return the exception information to the caller. Can the exception information be handled by the caller? Hope you provide the calling way and error message stack.


----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] liubao68 closed pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
liubao68 closed pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154


   


-- 
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: commits-unsubscribe@servicecomb.apache.org

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

[GitHub] [servicecomb-java-chassis] codecov-io commented on pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#issuecomment-748896390


   # [Codecov](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154?src=pr&el=h1) Report
   > Merging [#2154](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154?src=pr&el=desc) (19b5f7f) into [master](https://codecov.io/gh/apache/servicecomb-java-chassis/commit/323d5b7c406583867eacaa758b28e0fdcfcbe2c6?el=desc) (323d5b7) will **decrease** coverage by `0.01%`.
   > The diff coverage is `60.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154/graphs/tree.svg?width=650&height=150&src=pr&token=KXfDcr9rX2)](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2154      +/-   ##
   ============================================
   - Coverage     80.76%   80.75%   -0.02%     
     Complexity     1339     1339              
   ============================================
     Files          1463     1463              
     Lines         40142    40147       +5     
     Branches       3411     3411              
   ============================================
   - Hits          32420    32419       -1     
   - Misses         6255     6261       +6     
     Partials       1467     1467              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...ansport/rest/client/http/RestClientInvocation.java](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154/diff?src=pr&el=tree#diff-dHJhbnNwb3J0cy90cmFuc3BvcnQtcmVzdC90cmFuc3BvcnQtcmVzdC1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NlcnZpY2Vjb21iL3RyYW5zcG9ydC9yZXN0L2NsaWVudC9odHRwL1Jlc3RDbGllbnRJbnZvY2F0aW9uLmphdmE=) | `93.54% <60.00%> (-1.12%)` | `0.00 <0.00> (ø)` | |
   | [...pache/servicecomb/config/kie/client/KieClient.java](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154/diff?src=pr&el=tree#diff-ZHluYW1pYy1jb25maWcvY29uZmlnLWtpZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2VydmljZWNvbWIvY29uZmlnL2tpZS9jbGllbnQvS2llQ2xpZW50LmphdmE=) | `72.61% <0.00%> (-5.96%)` | `0.00% <0.00%> (ø%)` | |
   | [.../servicecomb/config/client/ConfigCenterClient.java](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154/diff?src=pr&el=tree#diff-ZHluYW1pYy1jb25maWcvY29uZmlnLWNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zZXJ2aWNlY29tYi9jb25maWcvY2xpZW50L0NvbmZpZ0NlbnRlckNsaWVudC5qYXZh) | `52.11% <0.00%> (-0.43%)` | `0.00% <0.00%> (ø%)` | |
   | [.../servicecomb/registry/discovery/DiscoveryTree.java](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154/diff?src=pr&el=tree#diff-Zm91bmRhdGlvbnMvZm91bmRhdGlvbi1yZWdpc3RyeS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2VydmljZWNvbWIvcmVnaXN0cnkvZGlzY292ZXJ5L0Rpc2NvdmVyeVRyZWUuamF2YQ==) | `100.00% <0.00%> (+3.50%)` | `0.00% <0.00%> (ø%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154?src=pr&el=footer). Last update [323d5b7...19b5f7f](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/2154?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] liuguangrong commented on a change in pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
liuguangrong commented on a change in pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#discussion_r547116599



##########
File path: transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/http/RestClientInvocation.java
##########
@@ -116,12 +118,17 @@ public void invoke(Invocation invocation, AsyncResponse asyncResp) throws Except
         filter.beforeSendRequest(invocation, requestEx);
       }
     }
+    List<HttpRestClientInvocationHook> httpRestClientInvocationHooks =
+            SPIServiceUtils.getAllService(HttpRestClientInvocationHook.class);
 
     clientRequest.exceptionHandler(e -> {
       invocation.getTraceIdLogger()
           .error(LOGGER, "Failed to send request, alreadyFailed:{}, local:{}, remote:{}, message={}.",
               alreadyFailed, getLocalAddress(), ipPort.getSocketAddress(),
               ExceptionUtils.getExceptionMessageWithoutTrace(e));
+      httpRestClientInvocationHooks.forEach(httpServerExceptionHandler -> {

Review comment:
       https://docs.servicecomb.io/java-chassis/zh_CN/references-handlers/intruduction/
   
   I found it didn't meet my requirements through experiments.
   
    My handler executes before the exception is returned, and does not return to handler after the exception is thrown.
   
   I can't get this exception message.
   
   Is there any other way




----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] liubao68 commented on a change in pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
liubao68 commented on a change in pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#discussion_r547244883



##########
File path: transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/http/RestClientInvocation.java
##########
@@ -116,12 +118,17 @@ public void invoke(Invocation invocation, AsyncResponse asyncResp) throws Except
         filter.beforeSendRequest(invocation, requestEx);
       }
     }
+    List<HttpRestClientInvocationHook> httpRestClientInvocationHooks =
+            SPIServiceUtils.getAllService(HttpRestClientInvocationHook.class);
 
     clientRequest.exceptionHandler(e -> {
       invocation.getTraceIdLogger()
           .error(LOGGER, "Failed to send request, alreadyFailed:{}, local:{}, remote:{}, message={}.",
               alreadyFailed, getLocalAddress(), ipPort.getSocketAddress(),
               ExceptionUtils.getExceptionMessageWithoutTrace(e));
+      httpRestClientInvocationHooks.forEach(httpServerExceptionHandler -> {

Review comment:
       ```
   public class MyHandler implements Handler {
   
     private static final Logger LOGGER = LoggerFactory.getLogger(MyHandler.class);
   
     public static final String SPLITPARAM_RESPONSE_USER_SUFFIX = "(modified by MyHandler)";
   
     @Override
     public void handle(Invocation invocation, AsyncResponse asyncResp) throws Exception {
       LOGGER.info("before");
   
       invocation.next(response -> {
            LOGGER.info("after");
       });
     }
   }
   ```
   
   You can find many Handler implementations in servicecomb-java-chassis project. 




----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] liubao68 commented on a change in pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
liubao68 commented on a change in pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#discussion_r547103818



##########
File path: transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/http/RestClientInvocation.java
##########
@@ -116,12 +118,17 @@ public void invoke(Invocation invocation, AsyncResponse asyncResp) throws Except
         filter.beforeSendRequest(invocation, requestEx);
       }
     }
+    List<HttpRestClientInvocationHook> httpRestClientInvocationHooks =
+            SPIServiceUtils.getAllService(HttpRestClientInvocationHook.class);
 
     clientRequest.exceptionHandler(e -> {
       invocation.getTraceIdLogger()
           .error(LOGGER, "Failed to send request, alreadyFailed:{}, local:{}, remote:{}, message={}.",
               alreadyFailed, getLocalAddress(), ipPort.getSocketAddress(),
               ExceptionUtils.getExceptionMessageWithoutTrace(e));
+      httpRestClientInvocationHooks.forEach(httpServerExceptionHandler -> {

Review comment:
       Maybe you can add a custom handler to fillful your requirements. 
   
   e.g. 'MyHandler implements Handler'
   
   servicecomb.handler.chain.Consumer.defaut:  your_current_handlers,myhandler
   
   myhandler can process the exception thrown in RestClientInvocation




----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] Volcannozzz commented on pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
Volcannozzz commented on pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#issuecomment-789019087


   Sorry, I have  got the answer from line 86, 125, 544  in `RestClientInvocation.java`.  


----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] liubao68 commented on pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
liubao68 commented on pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#issuecomment-774614878


   This handler throw exception , will hanldled in code block `after` in my example code


----------------------------------------------------------------
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.

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

[GitHub] [servicecomb-java-chassis] Volcannozzz edited a comment on pull request #2154: Edge's 490 exception is thrown and can be used for business processing

Posted by GitBox <gi...@apache.org>.
Volcannozzz edited a comment on pull request #2154:
URL: https://github.com/apache/servicecomb-java-chassis/pull/2154#issuecomment-789019087


   Sorry, maybe we can  got the answer from line 86, 125, 544  in `RestClientInvocation.java`.  
   so: 
   ```java
     InvokerUtils.reactiveInvoke("pojo", "InvokerEndpoint", "model", args, ClientModel.class, response -> {
       if (response.isFailed()) {
         // do something
       }
     });
   ```
   Is this okay？
   @liubao68 @liuguangrong 


----------------------------------------------------------------
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.

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