Michael Blow has posted comments on this change. Change subject: [NO ISSUE][OTH] Enable adding request channel close listener ......................................................................
Patch Set 5: (8 comments) https://asterix-gerrit.ics.uci.edu/#/c/1972/5/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IChannelCloseHandler.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IChannelCloseHandler.java: PS5, Line 26: Close close -vs- closed; let's pick one PS5, Line 29: closed closed -vs- close- let's pick one PS5, Line 36: taks task* PS5, Line 38: handle rename this to be less generic e.g. handleClosed, to enable classes to implement this multiple interfaces and have it be clear what this method is. https://asterix-gerrit.ics.uci.edu/#/c/1972/5/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IServlet.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IServlet.java: PS5, Line 50: null should we add HttpServer as a method parameter, and return server.getCHannelCloseHandler() here? it might make use of this less clunky, but no strong preference https://asterix-gerrit.ics.uci.edu/#/c/1972/5/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServer.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServer.java: PS5, Line 382: // Q. Why do we pass the handler? this was to enable extensions to choose a thread pool based on the request, which AFAIK none do https://asterix-gerrit.ics.uci.edu/#/c/1972/5/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java: PS5, Line 105: chek check* PS5, Line 103: // check if the servlet has a close handler first : IChannelCloseHandler channelCloseHandler = servlet.getChannelCloseHandler(); : // chek the if the server has a close handler : if (channelCloseHandler == null) { : channelCloseHandler = server.getChannelCloseHandler(); : } i think this gets cleaner if the servlet default impl delegates to server -- To view, visit https://asterix-gerrit.ics.uci.edu/1972 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42f1857c0158af6f447282cab8fbd600767b08d5 Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <dmitry.lycha...@couchbase.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Murtadha Hubail <mhub...@apache.org> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
