User code should use buffering for application specific reasons. May be 'bufferSize' argument for FSInputStream is flawed to start with.
Personally, I agree, but I think it's out of scope for this JIRA to fix that.
My impression is that main purpose of this patch is to reduce a copy. keeping the large buffer prohibits that.
That's true, but I think we need to thoroughly benchmark SequenceFile.Reader there, and do it in a separate JIRA. This one as it stands is not a breaking change, in that it should not reduce performance for any workload. Having a small internal buffer can potentially be breaking, so we should benchmark how big that break could be and weigh it vs the improvements.
Aside from making a smaller internal buffer, there are a couple other options that might be less "dangerous" - eg using a small buffer for the initial reads, then creating a new BufferedInputStream with a fresh buffer to start the data reads. This would get rid of the "misalignment" issue here. ChecksumFileSystem has this same problem, so introducing our own BufferedInputStream implementation that has some tricks to re-align its reads against the buffer.
Even when a sequencefile has very small records (avg < 1k?)
I've seen SequenceFiles used for even smaller records - down to a few bytes (eg IntWritable keys and values). Syscalls are cheap but not that cheap compared to an 8-byte copy. So, I don't think we should do optimizatinos that would destroy performance of this scenario.
...but not been able to see improvement. will verify if I am really running the patch.
Did you run this patch with a core jar that was compiled with
HADOOP-3205? To test, you need to do "ant -Dresolvers=internal mvn-install" from Common, with HADOOP-3205 applied. Then, in the HDFS tree, "ant -Dresolvers=internal clean-cache binary" to make sure it pulls your local common build.