[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 That's fine. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 How would you like your name to appear in the changelog? "Patch provided by Igal Sapir"? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 @violetagg I refactored the method to use `switch` statement as advised. https://github.com/apache/tomcat/pull/56/files#diff-5c721c838c78fa7c31f9eb62c27863ceR383 @ChristopherSchultz >I think the patch should include some TRACE-level logging, especially when catching the NumberFormatException. Added >If there is an unrecognized option string, it should at least result in a WARN message emitted to the log. I might even lobby for an exception to be thrown. Throwing IllegalArgumentException >There are options that seem to be in conflict with each other, if only in terminology: e.g. "async" and "sync" can be specified together, which would read like a bug in the config. If we add this then it should go into setChannelSendOptions() so that we also check if two conflicting integer options are passed then we warn/throw. I can add that, but it should be a separate ticket/PR IMO. >Splitting on a comma should include optional whitespace e.g. "\s,\s" as the pattern. Changed regex to `\s*,\s*` >The patch needs to include documentation for the string-based configuration options (webapps/docs/cluster-howto.html). Added >The javadoc in the patch needs to include the list of acceptable string option-names. Added >A small unit test wouldn't hurt, as well (just testing setChannelSendOptions(String) -> getChannelSendOptions:int would be fine). Added, **but I couldn't run the test case** in my IDE. getting an error `Error:(33, 16) java: package trailers does not exist` for `test\org\apache\coyote\http2\TestStream.java` and other files and I'm not sure yet how to resolve the dependencies. where is package `trailers` coming from? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 > Did you actually test it to see if it would give you the desired channel options? I actually did > A small unit test wouldn't hurt Will do --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 @isapir I appreciate your core patch as an initial effort. It looks like it will work. In order for it to be a good patch, it needs to be expanded, so a larger patch is appropriate. Did you actually test it to see if it would give you the desired channel options? A small unit test wouldn't hurt, as well (just testing setChannelSendOptions(String) -> getChannelSendOptions:int would be fine). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user violetagg commented on the issue: https://github.com/apache/tomcat/pull/56 > I was pointing out these examples while trying to figure out if you still require that I wrap each single statement if with braces, which would make the code less readable IMO. +1 for `switch` as you proposed earlier --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 @ChristopherSchultz That's exactly the kind of feedback that I was looking for. I will address the points to the best of my ability. Keep in mind that after those changes, the commit will be much larger and take longer to review, which raises the concerns that I mentioned in the mailing list. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 @violetagg Yes, I read that on http://tomcat.apache.org/getinvolved.html#Coding_Conventions I was pointing out these examples while trying to figure out if you still require that I wrap each single statement `if` with braces, which would make the code less readable IMO. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user violetagg commented on the issue: https://github.com/apache/tomcat/pull/56 > I noticed instances of the code where curly braces were omitted for single-statement ifs, Yep, we know that. If you take a look at the history you will see that typically when we prepare changes first make a change with formatting if we see that it is needed and then implement the change. We use - the standard Eclipse formatting - spaces only - lines up to 100 chars for code and 80 chars for javadoc, comments etc. - some additional settings for the compiler and javadoc http://svn.apache.org/repos/asf/tomcat/trunk/res/ide-support/eclipse/java-compiler-errors-warnings.txt --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 A few comments: 1. I think the patch should include some TRACE-level logging, especially when catching the NumberFormatException. 2. If there is an unrecognized option string, it should at least result in a WARN message emitted to the log. I might even lobby for an exception to be thrown. 3. There are options that seem to be in conflict with each other, if only in terminology: e.g. "async" and "sync" can be specified together, which would read like a bug in the config. 4. Splitting on a comma should include optional whitespace e.g. "\\s,*\\s*" as the pattern. 5. The patch needs to include documentation for the string-based configuration options (webapps/docs/cluster-howto.html). 6. The javadoc in the patch needs to include the list of acceptable string option-names. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #56: Convert Cluster Manager human-readable SendOptions names t...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 @violetagg Thank you for your feedback, I updated the `for`, `catch`, and one of the `if`'s. While looking into updating the rest of the `if` statements I noticed instances of the code where curly braces were omitted for single-statement `if`s, e.g. https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L256 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L424 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L470 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L500 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L503 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L537 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L546 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L564 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L565 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L570 https://github.com/isapir/tomcat/blob/e51512fdc062ffd07a3dd323cba76ab057d23322/java/org/apache/catalina/ha/tcp/SimpleTcpCluster.java#L621 Anyway, it seemed like I shouldn't even continue in that file. If you want to enforce these styles then perhaps it'd be better to run the source code through a code formatter. Thanks, Igal --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
