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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
