[
https://issues.apache.org/jira/browse/SLIDER-1124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15293826#comment-15293826
]
Gour Saha commented on SLIDER-1124:
-----------------------------------
[~billie.rinaldi] the patch looks good. Couple of comments -
1.
h6. PortScanner.java
bq. throw new BadConfigException("Bad port range: " + input);
It might be more useful if we print exactly which range caused this bad config
exception if we print the range instead of the entire input string, or maybe
print both. Something like this -
{code}
throw new BadConfigException("Bad port range: " + range + " in input: " +
input);
{code}
2.
If the user sets an empty string or string with only spaces (say input is "" or
" "), I think the code will throw a BadConfigException. Is that what we want
here?
> If unparsable port range is specified, Slider AM PortScanner.java
> setPortRange() should throw exception - add else part
> ------------------------------------------------------------------------------------------------------------------------
>
> Key: SLIDER-1124
> URL: https://issues.apache.org/jira/browse/SLIDER-1124
> Project: Slider
> Issue Type: Improvement
> Components: appmaster
> Affects Versions: Slider 0.80
> Reporter: Manoj Samel
> Assignee: Billie Rinaldi
> Fix For: Slider 0.91
>
> Attachments: SLIDER-1124.1.patch
>
>
> {noformat}
> The issue was discovered when a JSON was generated with IDE and instead of
> "-", it somehow inserted a similar looking but different character sequence.
> In this case, Slider AM fails to start with following ERROR
> [main] ERROR main.ServiceLauncher - No available ports found in configured
> range {}
> Above error gives a impression that all available ports in specified range
> were some how not available; which is not the case.
> Json was updated using IDE, and while at first glance it looks like
> "32201-33100", it was really "32201–33100" . The character in the second case
> is not a "-" but actually three characters that together appear somewhat like
> it (but its wider and lower than - ).
> So this is neither a "," separated list or "-" range as the code expects and
> it errors out.
> It would be useful if such "bad" range is caught up earlier with clearer
> message like invalid or unparsable port range specified.
> Looking at the code
> SliderAppMaster.java buildPortScanner() reads the key KEY_ALLOWED_PORT_RANGE
> and passes the associated value to portScanner.setPortRange().
> In PortScanner.java setPortRange() , it first tries to split on "," or else
> tries to split on "-". However, there is no "else" part if it does not finds
> the "-" pattern (which will happen in above case). Since there is no else
> part, there is no exception etc. thrown at this point and
> this.remainingPortsToCheck gets set to a empty set, resulting in more obscure
> error later in getAvailablePortViaPortArray().
> I think it would be good to have a "else" part added to range matchers below
> and a exception with input text thrown at that point - so the misconfigured
> value will be obvious
> Matcher m = SINGLE_NUMBER.matcher(range.trim());
> if (m.find()) {
> inputPorts.add(Integer.parseInt(m.group()));
> } else {
> m = NUMBER_RANGE.matcher(range.trim());
> if (m.find()) {
> } // else is missing ..... Add with a exception ???
> {noformat}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
