[GitHub] tomcat issue #56: Convert Cluster Manager human-readable channelSendOptions ...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 Not quite yet. Let's see if we can get it closed automatically through the "proper" channels. --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 Should I close it manually? --- 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 channelSendOptions ...
Github user violetagg commented on the issue: https://github.com/apache/tomcat/pull/56 There aren't any replications since yesterday :( --- 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 channelSendOptions ...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 I'm not sure why the commit message hasn't closed this issue. Any thoughts on why that might be? --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 Thank you ð --- 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 channelSendOptions ...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 Committed to tomcat-trunk in ASF svn r1799462. Back-ports to follow. --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 @ChristopherSchultz can we merge this please? --- 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 channelSendOptions ...
Github user KeiichiFujino commented on the issue: https://github.com/apache/tomcat/pull/56 I confirmed. there is no problem. Thanks. --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 @KeiichiFujino p.s. This is the last commit that addresses the issues above: https://github.com/apache/tomcat/pull/56/commits/192d2eb13e9ec9448e183dcc3b166f7d3577c250 --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 @KeiichiFujino Thank you for your prompt review. >you need to add BackupManager mbean definitions ... in org/apache/catalina/ha/session/mbeans-descriptors.xml Thank you for providing the path. I was looking for that and didn't find it easily. >There is a typo in ... "channelStartOptions name." -> "channelSendOptions name." Corrected. I actually copied and pasted it from your comment above -- https://github.com/apache/tomcat/pull/56#issuecomment-307055014 -- and simply added new line characters, but I should have caught that ;) Can you please confirm that as far as you're concerned this can now be merged? @ChristopherSchultz is awaiting your approval. Thank you :) --- 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 channelSendOptions ...
Github user KeiichiFujino commented on the issue: https://github.com/apache/tomcat/pull/56 There are two comments. you need to add BackupManager mbean definitions. ``` ``` in org/apache/catalina/ha/session/mbeans-descriptors.xml There is a typo in ``` ``` "channelStartOptions name." -> "channelSendOptions name." --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 @ChristopherSchultz I updated the patch according to your feedback: https://github.com/apache/tomcat/pull/56/commits/f0c3968570f70823d30cf18144e82591a7ee0cee I was actually trying at first to get it to work with a bit shift but when it didn't work I resorted to `Math.pow(2, bit)` -- bit shift is definitely better, though it's possible that `Math.pow(2, x)` utilizes shift in the JRE anyway. In any event, great feedback, thanks! --- 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 channelSendOptions ...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 Thanks @Igal for your patience with this PR review. I know we've been a bit of a pain. But this work will make the contribution that much more useful. --- 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 channelSendOptions ...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 I'm waiting on a final review from @KeiichiFujino before I commit. In the meantime there are 2 `@return` javadoc annotations with no value. Could you complete those? --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 I'll change it. Please wait. --- 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 channelSendOptions ...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 @isapir FYI `1 << bit` is the same as `(int)Math.pow(2, bit)` and probably slightly more efficient. Better yet, it's less code to read. No particular reason to change the patch. I'm already happy with what we've got. --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 @KeiichiFujino @ChristopherSchultz I added: * a method to translate the int value to a string list: https://github.com/apache/tomcat/pull/56/files#diff-5c721c838c78fa7c31f9eb62c27863ceR439 * `public String getChannelSendOptionsName()` https://github.com/apache/tomcat/pull/56/files#diff-8e81c1ebb6efdca13082a2798a305251R403 * a read-only `channelSendOptionsName` in mbeans-descriptor https://github.com/apache/tomcat/pull/56/files#diff-9dca3596f50abfe90c0ed9b54d4e2092R32 * a test-case that checks the string representation https://github.com/apache/tomcat/pull/56/files#diff-a638e8120005efa2cccdf63066b7fde5R53 I believe that this addresses all of the issues with this PR. --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 >Even if updating the type of channelStartOptions from int to String in mbeans-descriptor, it will display int format values. Because the channelSendOptions is always set to int format. if you set channelSendOptions = "asynchronous", 8 will be displayed via JMX. Right, because we are not changing the way that the value is stored, we are only adding a way to make it easier to set it. You will still be able to change the value (not sure if that's even permitted at runtime) via JMX with either `String` or `int` value, but it will be displayed in its `int` form. This is the way that it is already implemented for `channelStartOptions` Which is also an `int`, but specified as `String` in the mbeans-descriptor, and TBH, it will be inconsistent if other similar values will be displayed as `int` and this one will be displayed as `String`. I can write a method to translate back from `int` to `String`, but IMO this is really unnecessary, as it is an advanced option which you don't normally change at runtime, and if you need to troubleshoot an issue then you know what value you expect to see. Again, setting the value via JMX should work now with either `String` or `int` input. It's only the way that it is displayed that remains an `int`. --- 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 channelSendOptions ...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 @KeiichiFujino Okay, so we'd have a synthetic mbean attribute that we can re-construct from the int value we store internally? Or, do you think we should save the string that was used in configuration? --- 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 channelSendOptions ...
Github user KeiichiFujino commented on the issue: https://github.com/apache/tomcat/pull/56 Sorry for the late reply. I do not think it work as expected. Even if updating the type of channelStartOptions from int to String in mbeans-descriptor, it will display int format values. Because the channelSendOptions is always set to int format. if you set channelSendOptions = "asynchronous", 8 will be displayed via JMX. For example, The channelSendOptions in the String format specified by the argument must be saved under a different name(e.g. channelSendOptionsName) and published to JMX. Then add the following definition to mbeans-descriptor. ` ` --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 Can we merge this or do we need to wait for @KeiichiFujino 's input? I believe that I addressed all of the issues that he pointed out. --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 >If you configure channelSendOptions in string form, you should also expose it to JMX in string format. You can see org/apache/catalina/ha/tcp/mbeans-descriptors.xml I updated the type from `int` to `String`. I see that the current implementation does the same for `channelStartOptions` which is also an `int`, but set as a `String` in the mbeans-descriptor, so I expect it to work the same even though TBH I have not tested with JMX. --- 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 channelSendOptions ...
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/56 >It is necessary to fix configuration docs (webapps/config/cluster.xml). Added >The patch needs to include fix for mapSendOptions. BackupManager use mapSendOptions instead of channelSendOptions. Added --- 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 channelSendOptions ...
Github user ChristopherSchultz commented on the issue: https://github.com/apache/tomcat/pull/56 @KeiichiFujino Is this patch acceptable as-is, with additional patches for your items listed above? I think it's okay to add this feature only to channelSendOptions as a first commit. Adding mapSendOptions and JMX support can come later. Okay? --- 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 channelSendOptions ...
Github user KeiichiFujino commented on the issue: https://github.com/apache/tomcat/pull/56 Hi, some comments : - SimpleTcpCluster exposes the configuration via JMX. If you configure channelSendOptions in string form, you should also expose it to JMX in string format. You can see org/apache/catalina/ha/tcp/mbeans-descriptors.xml - It is necessary to fix configuration docs (webapps/config/cluster.xml). - The patch needs to include fix for mapSendOptions. BackupManager use mapSendOptions instead of channelSendOptions. --- 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
