Details

Description

The token's service field must (currently) be set to "ip:port". All the producers of a token are independently building the service string. This should be done via a common method to reduce the chance of error, and to facilitate the field value being easily changed in the (near) future.

I see only one issue with changing the setService API to take InetSocketAddress. It forces to set ip/host/port in the service. If we later decide to store something else in the service for example, uri with scheme, we will have to change it again.
I would recommend to keep setService API unchanged, however a utility method in SecurityUtil to construct DT service from a socket address is good.

Jitendra Nath Pandey
added a comment - 08/Aug/11 17:55 I see only one issue with changing the setService API to take InetSocketAddress. It forces to set ip/host/port in the service. If we later decide to store something else in the service for example, uri with scheme, we will have to change it again.
I would recommend to keep setService API unchanged, however a utility method in SecurityUtil to construct DT service from a socket address is good.

Per the title of this jira, that is the goal. Every token producer is required to use the same service field format to comply with the format needed by the token's selector. Currently every token producer has a copy-n-paste chunk selector's code to construct the service format. If the selector and the producer get out of sync, there's a big problem.

If we later decide to store something else in the service for example, uri with scheme, we will have to change it again.

Please elaborate? This appears as a non-sequitur since token producers that choose to use a URI (someday), for instance, will require a change in either case.

I think is your suggestion? It's incrementally better, but continues to require a copy-n-paste in each token producer, and every token producer continues to have intimate knowledge of the service format.

Daryn Sharp
added a comment - 08/Aug/11 19:22 It forces to set ip/host/port in the service.
Per the title of this jira, that is the goal. Every token producer is required to use the same service field format to comply with the format needed by the token's selector. Currently every token producer has a copy-n-paste chunk selector's code to construct the service format. If the selector and the producer get out of sync, there's a big problem.
If we later decide to store something else in the service for example, uri with scheme, we will have to change it again.
Please elaborate? This appears as a non-sequitur since token producers that choose to use a URI (someday), for instance, will require a change in either case.
Here is the evolution from the original code of:
token.setService( new Text(addr.getAddress().getHostAddress() + ":" + addr.getPort()));
selector.selectToken( new Text(addr.getAddress().getHostAddress() + ":" + addr.getPort()), tokens);
I think is your suggestion? It's incrementally better, but continues to require a copy-n-paste in each token producer, and every token producer continues to have intimate knowledge of the service format.
token.setService(SecurityUtil.buildDTAuthority(addr));
selector.selectToken(SecurityUtil.buildDTAuthority(addr), tokens);
The patch applies another layer of abstraction. The format is privatized to the token, instead of publicly diffused over all the tokens in hadoop.
token.setService(addr);
selector.selectToken(Token.createService(addr), tokens);
// I removed this due to your earlier concerns in the parent jira
// selector.selectToken(addr, tokens);
Do you believe this a persuasive case for the patch?

Daryn and I discussed it at length, the conclusions were:
It is better to treat service as opaque, as in current Token API.
A convenience method to set service using InetSocketAddress is useful.
Therefore we add a method in SecurityUtil that takes a token and an InetSocketAddress, constructs a service and sets it in the token. And the Token API remains unchanged.

Jitendra Nath Pandey
added a comment - 08/Aug/11 23:48 Daryn and I discussed it at length, the conclusions were:
It is better to treat service as opaque, as in current Token API.
A convenience method to set service using InetSocketAddress is useful.
Therefore we add a method in SecurityUtil that takes a token and an InetSocketAddress, constructs a service and sets it in the token. And the Token API remains unchanged.

[exec] +1 overall.[exec][exec] +1 @author. The patch does not contain any @author tags.[exec][exec] +1 tests included. The patch appears to include 15 new or modified tests.[exec][exec] +1 javadoc. The javadoc tool did not generate any warning messages.[exec][exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.[exec][exec] +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.[exec]