[
https://issues.apache.org/jira/browse/MAPREDUCE-1122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12890205#action_12890205
]
Vinod K V commented on MAPREDUCE-1122:
--------------------------------------
I started looking at this patch. Its BIG, and streaming isn't exactly my
'home-ground' :) I had to spend quite some time reviewing it. Please bear with
me, we will need to go through some iterations, at-least one more big one, to
get this close.
- First up, the patch needs some merging to be done to accommodate recent
commits in streaming.
- It'd be really good if we can separate the new classes into new packages,
library classes into a lib package and implementation classes to an impl
package?
- There are two ways of handing the skipping of bad records in the new api -
(1) put the code in place and document that it isn't supported yet so that
whenever MAPREDUCE-1932 moves in skipping automatically works in streaming or
(2) remove the code altogether and create a child issue of MAPREDUCE-1932 for
streaming. Looks like you intended to do (2) but I do see some (dead) code
related to skipping in the new api classes, for e.g. StreamingMapper. We should
either chose (1) completely or (2) completely.
Otherwise the overall functionality looks good to me, correctness including.
Just some minor comments.
StreamingMapper.java
- This log statement is new and we are doing for every key. Too aggressive?
LOG.info("input " + key + " "+ value);
- Difference in logging compared to old PipeMapred class when exceptions
happen in map.
- Missing @Override annotation for methods overridden.
StreamingReducer.java
- Not logging exit code when exceptions happen in reduce. Used to be the case
in old code.
- Missing @Override annotation for methods overridden.
How about passing configuration configuration to InputWriter.initialize() and
let TextInputWriter/TextOutputReader maintain themselves the key/vaule
separators and related information instead of polluting StreamingMapper and
StreamingReducer?
StreamingCombiner
- Missing @Override annotation for method overridden.
Autoinputformat2
- No configure method like in AutoInputFormat?
- Name? Once we move the lib classes to a new package, this class's name can
stay the same old AutoInputFormat.
StreamingXmlRecordReader.java
- Log.info statement in init() bears the wrong (parent) class name.
- nextKeyValue() should be synchronized? In old api it was.
StreamingBaseRecordReader.java
- getStatus() has changed w.r.t printing 'pos' also when compared to the older
StreamBaseRecordReader.java
StreamJob.java
- bq. Removes deprectaed StreamJob(String[] argv, boolean mayExit);
Just checking. Is the compatibility left in one release?
- Same for StreamJob.go()?
- boolean isOldIF argument to setOutputFormat is not used at all.
- Cluster and hence StreamJob never close client connection themselves at all!
( May be another ticket)
TestStreamingStatus:
- + //testStreamJob(false);// nonempty input
Commented intentionally, in testReporing()?
- Comments from +262 to +265 are no longer valid, right?
TrApp.java
- Some expect() and expectDefined() calls are dropped. I could understand why
the ones related to output format are dropped to accommodate testing both new
and old apis. But removing of the checks related to input file and file length
didn't make sense to me.
TaskInputOuputContextImpl.
- The changes here were a surprise to me. Should be related to MAPREDUCE-1905.
Are you incorporating that here, or just kept them in the patch for running. If
it's the later, please provide a patch without these changes. If it is the
former, we will need to include the testcase from there too.
Miscellaneous comments:
- It's right time for us to mark all the touched classes/interfaces according
to the classification taxonomy.
- Should we make the initialize methods in InputWriter and OutputReader
abstract now?
- TestStreamingAPICompatibility class needs some javadoc.
- TODO: In the end we need to be sure tests pass with LinuxTaskController as
well. Please do this with your next patch if you'ven't already.
> streaming with custom input format does not support the new API
> ---------------------------------------------------------------
>
> Key: MAPREDUCE-1122
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-1122
> Project: Hadoop Map/Reduce
> Issue Type: Bug
> Components: contrib/streaming
> Affects Versions: 0.20.1
> Environment: any OS
> Reporter: Keith Jackson
> Assignee: Amareshwari Sriramadasu
> Fix For: 0.22.0
>
> Attachments: patch-1122.txt
>
>
> When trying to implement a custom input format for use with streaming, I have
> found that streaming does not support the new API,
> org.apache.hadoop.mapreduce.InputFormat, but requires the old API,
> org.apache.hadoop.mapred.InputFormat.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
