Alejandro Abdelnur
added a comment - 31/May/12 16:24 Instead using explicit parameters we could introduces a Parameters parameter which is seeded by a Jersey @Provider. This provider would also do the corresponding type parsing.
This work will simplify the implementation of HttpFS delegation tokens support HDFS-3113

Finally, regarding sharing Param code with webhdfs. The idea is, once that webhdfs and httpfs are 100% equivalent from a functional perspective (HDFS-3113 & HDFS-3509 would achieve that), then we can tackle unify the code (HDFS-2645).

Alejandro Abdelnur
added a comment - 25/Jun/12 17:09 Thx Eli.
The attached patch removes the commented return in the testcase (there was only one occurrence of this).
The Parameter class, while a simple wrapper on Map, given its generic method simplifies access to parameter values significantly, making the code cleaner, for example:
Using Parameters:
String doAs = params.get(DoAsParam.NAME, DoAsParam.class);
Using Map:
String doAs = ((DoAsParam.class)map.get(DoAsParam.NAME, DoAsParam.class)).value();
And it also removes access to all the Map API which are not relevant for this use (if we use Map we'd had to wrap it in an unmodifiable MAP to avoid).
Regarding Using Guava ImmutableMap.of(), I'm getting similar warnings.
Finally, regarding sharing Param code with webhdfs. The idea is, once that webhdfs and httpfs are 100% equivalent from a functional perspective ( HDFS-3113 & HDFS-3509 would achieve that), then we can tackle unify the code ( HDFS-2645 ).

Makes sense wrt the Parameter class. I don't follow the logic wrt having two Param classes, ie we can start sharing code before we're 100% functionally equivalent. Not a blocker for this issue since we already have a duplicated Param class.

Eli Collins
added a comment - 26/Jun/12 17:54 What's the warning?
Makes sense wrt the Parameter class. I don't follow the logic wrt having two Param classes, ie we can start sharing code before we're 100% functionally equivalent. Not a blocker for this issue since we already have a duplicated Param class.

Regarding the duplication of Parameter class, I think the reason for the duplication is that Webhdfs is tightly coupled with HDFS code (within the same maven module) while HttpFS is decoupled and could (in theory) be used without HDFS itself in the classpath. As part of HDFS-2645 all this dup code would go away.

Alejandro Abdelnur
added a comment - 26/Jun/12 18:40 On the warning:
Offending source:
...
private static final Map<Enum, Class <Param<?>>[]> PARAMS_DEF =
new HashMap<Enum, Class <Param<?>>[]>();
static {
PARAMS_DEF.put(Operation.OPEN,
new Class []{DoAsParam.class, OffsetParam.class, LenParam.class});
...
[WARNING] /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build/trunk/hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSParametersProvider.java:[48,6] [unchecked] unchecked conversion
found : java.lang. Class []
required: java.lang. Class <org.apache.hadoop.lib.wsrs.Param<?>>[]
Regarding the duplication of Parameter class, I think the reason for the duplication is that Webhdfs is tightly coupled with HDFS code (within the same maven module) while HttpFS is decoupled and could (in theory) be used without HDFS itself in the classpath. As part of HDFS-2645 all this dup code would go away.

Eli Collins
added a comment - 26/Jun/12 22:44 Yea, I don't think there's a workaround since java doesn't allow generic array creation. In this case we know the type statically so you could put the puts in a static method with @SuppressWarnings(
{"unchecked"}
), but IMO that's harder to read so the current code in your patch is better. +1 to the latest patch.