You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@eventmesh.apache.org by "sanskar-thakur (via GitHub)" <gi...@apache.org> on 2023/03/28 16:45:52 UTC
[GitHub] [eventmesh] sanskar-thakur opened a new pull request, #3547: [Enhancement] Method manually handles closing an auto-closeable resource [MetricsHandler] #3009
sanskar-thakur opened a new pull request, #3547:
URL: https://github.com/apache/eventmesh/pull/3547
Fixes #3009 .
### Modifications
Replaced finally block with try-with-resources.
### Documentation
- Does this pull request introduce a new feature? **(no)**
- If yes, how is the feature documented? **(not applicable)**
- If a feature is not applicable for documentation, explain why? **(bug fix)**
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation **(not applicable)**
--
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] Alonexc commented on pull request #3547: [Enhancement] Method manually handles closing an auto-closeable resource [MetricsHandler] #3009
Posted by "Alonexc (via GitHub)" <gi...@apache.org>.
Alonexc commented on PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#issuecomment-1488185723
Here you may still have to use try-with-finally, using try-with-resources will be due to the scope of the variable, here in the catch there is out.write(result.getBytes()); this line of code is reported as an error. Do you have a better suggestion?
--
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] mxsm commented on a diff in pull request #3547: [Enhancement] Method manually handles closing an auto-closeable resource [MetricsHandler] #3009
Posted by "mxsm (via GitHub)" <gi...@apache.org>.
mxsm commented on code in PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#discussion_r1151317018
##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/admin/handler/MetricsHandler.java:
##########
@@ -134,15 +132,8 @@ void get(HttpExchange httpExchange) throws IOException {
String result = JsonUtils.toJSONString(error);
byte[] bytes = result.getBytes(Constants.DEFAULT_CHARSET);
httpExchange.sendResponseHeaders(500, bytes.length);
- out.write(bytes);
- } finally {
- if (out != null) {
- try {
- out.close();
- } catch (IOException e) {
- log.warn("out close failed...", e);
- }
- }
+
+ log.error(result, e);
Review Comment:
Remove ` log.error(result, e);`? and not remove `out.write(bytes);`
--
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] sanskar-thakur commented on a diff in pull request #3547: [ISSUE #3009] Method manually handles closing an auto-closeable resource [MetricsHandler]
Posted by "sanskar-thakur (via GitHub)" <gi...@apache.org>.
sanskar-thakur commented on code in PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#discussion_r1155125178
##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/admin/handler/HTTPClientHandler.java:
##########
@@ -95,7 +95,8 @@ void delete(HttpExchange httpExchange) throws IOException {
Error error = new Error(e.toString(), stackTrace);
String result = JsonUtils.toJSONString(error);
httpExchange.sendResponseHeaders(500, result.getBytes().length);
Review Comment:
Sure, updated in latest commit.
--
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] mytang0 commented on pull request #3547: [Enhancement] Method manually handles closing an auto-closeable resource [MetricsHandler] #3009
Posted by "mytang0 (via GitHub)" <gi...@apache.org>.
mytang0 commented on PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#issuecomment-1489796472
Linked to #3544, PR overlapped.
--
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] mytang0 commented on a diff in pull request #3547: [Enhancement] Method manually handles closing an auto-closeable resource [MetricsHandler] #3009
Posted by "mytang0 (via GitHub)" <gi...@apache.org>.
mytang0 commented on code in PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#discussion_r1152649875
##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/admin/handler/HTTPClientHandler.java:
##########
@@ -95,7 +95,8 @@ void delete(HttpExchange httpExchange) throws IOException {
Error error = new Error(e.toString(), stackTrace);
String result = JsonUtils.toJSONString(error);
httpExchange.sendResponseHeaders(500, result.getBytes().length);
Review Comment:
Please help simplify, replace with 'httpExchange.sendResponseHeaders(500, 0);'
--
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
Re: [PR] [ISSUE #3009] Method manually handles closing an auto-closeable resource [MetricsHandler] (eventmesh)
Posted by "scwlkq (via GitHub)" <gi...@apache.org>.
scwlkq commented on PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#issuecomment-1902124690
@Pil0tXia this pr seems to be merged after the second reviewer reviews
--
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
Re: [PR] [ISSUE #3009] Method manually handles closing an auto-closeable resource [MetricsHandler] (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo closed pull request #3547: [ISSUE #3009] Method manually handles closing an auto-closeable resource [MetricsHandler]
URL: https://github.com/apache/eventmesh/pull/3547
--
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] sanskar-thakur commented on a diff in pull request #3547: [Enhancement] Method manually handles closing an auto-closeable resource [MetricsHandler] #3009
Posted by "sanskar-thakur (via GitHub)" <gi...@apache.org>.
sanskar-thakur commented on code in PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#discussion_r1152173413
##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/admin/handler/MetricsHandler.java:
##########
@@ -134,15 +132,8 @@ void get(HttpExchange httpExchange) throws IOException {
String result = JsonUtils.toJSONString(error);
byte[] bytes = result.getBytes(Constants.DEFAULT_CHARSET);
httpExchange.sendResponseHeaders(500, bytes.length);
- out.write(bytes);
- } finally {
- if (out != null) {
- try {
- out.close();
- } catch (IOException e) {
- log.warn("out close failed...", e);
- }
- }
+
+ log.error(result, e);
Review Comment:
because "out" stream went out of scope in catch block. Hence, the code would not compile.
--
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] sanskar-thakur commented on a diff in pull request #3547: [Enhancement] Method manually handles closing an auto-closeable resource [MetricsHandler] #3009
Posted by "sanskar-thakur (via GitHub)" <gi...@apache.org>.
sanskar-thakur commented on code in PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#discussion_r1152216723
##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/admin/handler/MetricsHandler.java:
##########
@@ -134,15 +132,8 @@ void get(HttpExchange httpExchange) throws IOException {
String result = JsonUtils.toJSONString(error);
byte[] bytes = result.getBytes(Constants.DEFAULT_CHARSET);
httpExchange.sendResponseHeaders(500, bytes.length);
- out.write(bytes);
- } finally {
- if (out != null) {
- try {
- out.close();
- } catch (IOException e) {
- log.warn("out close failed...", e);
- }
- }
+
+ log.error(result, e);
Review Comment:
> Here you may still have to use try-with-finally, using try-with-resources will be due to the scope of the variable, here in the catch there is out.write(result.getBytes()); this line of code is reported as an error. Do you have a better suggestion?
From Java 9 onwards, I think this could be solved with declaring out stream outside try block :
OutputStream out = httpExchange.getResponseBody();
try (out) {
} catch (Exception e) {
// use out here.
}
But with Java 8, proposing use of nested try blocks so that out of scope variable could be used.
Also solving compilation issue due to issue#3007 and issue#3008.
Please let me know your inputs.
--
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
Re: [PR] [ISSUE #3009] Method manually handles closing an auto-closeable resource [MetricsHandler] (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#issuecomment-1902137592
Please resolve conflicts.
--
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
Re: [PR] [ISSUE #3009] Method manually handles closing an auto-closeable resource [MetricsHandler] (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on PR #3547:
URL: https://github.com/apache/eventmesh/pull/3547#issuecomment-2014742241
completed in #4800
--
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