Phabricator link put up by viraj : https://reviews.facebook.net/D12063

I'm +1 on this. Ashutosh Chauhan, could you please check this?

I don't see the feedback provided on https://reviews.facebook.net/D12063#comment-2 & https://reviews.facebook.net/D12063#comment-3 getting addressed in latest patch.

Hi Ashutosh are you seeing the latest revision? Viraj

Ashutosh, FYI, I had issues with the diff between versions of diffs by Phabricator not showing correct diffs. I had to download the unified patch Viraj put up (HIVE-4331.patch) to see the changes.

Per discussions between Viraj, Ashutosh and myself, we need a couple more changes on this patch. Viraj is currently respinning his patch with these changes:

Line numbers addendum to Ashutosh's original review:

73 : ok

105 : can be improved by doing inner-if once, but will not require respinning by itself. If we have to respin though, we should do this.

configureTableJobProperties : HBaseConfiguration.addHbaseResources seems to take Configuration, which makes it unnecessary to create copyOfConf. In this case, avoid copy, and avoid the if two lines later as well, where we check if it's an instance of JobConf or not. Respin.

> possible to change addHBaseDelegationToken to accept Configuration, and instantiate a JobContext to pass into User.getCurrent().obtainAuthTokenForJob(conf,job) form.

341 : similar comment

Refactor to do similar things outside the if(), and only different things inside the if-else for inputjobprops/outputjobprops

100 : fine

123 : replace concurrenthashmap with string, keep in hivefileformatutils

Hi Sushanth and Ashutosh,
Thanks for taking time to discuss the changes at length. I have not attached the patch on the Hive side to take care of things as suggested in the previous comment. We are skipping phabricator. I will make this patch available and let the PRE-COMMIT take over.
Thanks again
Viraj

New patch on the hive side to take care of the comments on 9/10/2013

+1

Overall: -1 no tests executed

Here are the results of testing the latest attachment:
https://issues.apache.org/jira/secure/attachment/12602666/HIVE-5260.patch

Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/702/testReport
Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/702/console

Messages:

Executing org.apache.hive.ptest.execution.PrepPhase
Tests failed with: NonZeroExitCodeException: Command 'bash /data/hive-ptest/working/scratch/source-prep.sh' failed with exit status 1 and output '+ [[ -n '' ]]
+ export 'ANT_OPTS=-Xmx1g -XX:MaxPermSize=256m -Dhttp.proxyHost=localhost -Dhttp.proxyPort=3128'
+ ANT_OPTS='-Xmx1g -XX:MaxPermSize=256m -Dhttp.proxyHost=localhost -Dhttp.proxyPort=3128'
+ cd /data/hive-ptest/working/
+ tee /data/hive-ptest/logs/PreCommit-HIVE-Build-702/source-prep.txt
+ mkdir -p maven ivy
+ [[ svn = \s\v\n ]]
+ [[ -n '' ]]
+ [[ -d apache-svn-trunk-source ]]
+ [[ ! -d apache-svn-trunk-source/.svn ]]
+ [[ ! -d apache-svn-trunk-source ]]
+ cd apache-svn-trunk-source
+ svn revert -R .
++ egrep -v '^X|^Performing status on external'
++ awk '{print $2}'
++ svn status --no-ignore
+ rm -rf
+ svn update

Fetching external item into 'hcatalog/src/test/e2e/harness'
External at revision 1522097.

At revision 1522097.
+ patchCommandPath=/data/hive-ptest/working/scratch/smart-apply-patch.sh
+ patchFilePath=/data/hive-ptest/working/scratch/build.patch
+ [[ -f /data/hive-ptest/working/scratch/build.patch ]]
+ chmod +x /data/hive-ptest/working/scratch/smart-apply-patch.sh
+ /data/hive-ptest/working/scratch/smart-apply-patch.sh /data/hive-ptest/working/scratch/build.patch
The patch does not appear to apply with p0 to p2
+ exit 1
'

This message is automatically generated.

The previous message was caused because the git apply patch worked well but the patch -p1 command does not. Attaching another patch.
Viraj

patch which will apply cleanly

I'm +1 on the latest changes - they are in accordance with the requirements discussion we had yesterday.

Overall: +1 all checks pass

Here are the results of testing the latest attachment:
https://issues.apache.org/jira/secure/attachment/12602727/HIVE-5260.2.patch

SUCCESS: +1 3091 tests passed

Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/709/testReport
Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/709/console

Messages:

Executing org.apache.hive.ptest.execution.PrepPhase
Executing org.apache.hive.ptest.execution.ExecutionPhase
Executing org.apache.hive.ptest.execution.ReportingPhase

This message is automatically generated.

Committed to trunk. Thanks, Viraj!

Thanks Ashutosh and Sushanth for your help.
Viraj

FAILURE: Integrated in Hive-trunk-hadoop2-ptest #96 (See https://builds.apache.org/job/Hive-trunk-hadoop2-ptest/96/)
HIVE-5260 : Introduce HivePassThroughOutputFormat that allows Hive to use general purpose OutputFormats instead of HiveOutputFormats in StorageHandlers (Viraj Bhat via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1522574)

• /hive/trunk/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java
• /hive/trunk/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HiveHBaseTableOutputFormat.java
• /hive/trunk/hbase-handler/src/test/results/positive/external_table_ppd.q.out
• /hive/trunk/hbase-handler/src/test/results/positive/hbase_binary_storage_queries.q.out
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HiveFileFormatUtils.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HivePassThroughOutputFormat.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HivePassThroughRecordWriter.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/CreateTableDesc.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java

FAILURE: Integrated in Hive-trunk-hadoop1-ptest #163 (See https://builds.apache.org/job/Hive-trunk-hadoop1-ptest/163/)
HIVE-5260 : Introduce HivePassThroughOutputFormat that allows Hive to use general purpose OutputFormats instead of HiveOutputFormats in StorageHandlers (Viraj Bhat via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1522574)

• /hive/trunk/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java
• /hive/trunk/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HiveHBaseTableOutputFormat.java
• /hive/trunk/hbase-handler/src/test/results/positive/external_table_ppd.q.out
• /hive/trunk/hbase-handler/src/test/results/positive/hbase_binary_storage_queries.q.out
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HiveFileFormatUtils.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HivePassThroughOutputFormat.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HivePassThroughRecordWriter.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/CreateTableDesc.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java

Thejas M Nair/Ashutosh Chauhan : Could we get this on to 0.12 as well? We need it there to be able to deprecate the hbase storage handler as of 0.12.

FAILURE: Integrated in Hive-trunk-h0.21 #2327 (See https://builds.apache.org/job/Hive-trunk-h0.21/2327/)
HIVE-5260 : Introduce HivePassThroughOutputFormat that allows Hive to use general purpose OutputFormats instead of HiveOutputFormats in StorageHandlers (Viraj Bhat via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1522574)

• /hive/trunk/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java
• /hive/trunk/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HiveHBaseTableOutputFormat.java
• /hive/trunk/hbase-handler/src/test/results/positive/external_table_ppd.q.out
• /hive/trunk/hbase-handler/src/test/results/positive/hbase_binary_storage_queries.q.out
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HiveFileFormatUtils.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HivePassThroughOutputFormat.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HivePassThroughRecordWriter.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/CreateTableDesc.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java

Sushanth I think the patch to apply to Hive 0.12 as well. I would be good to have this in Hive 0.12

ABORTED: Integrated in Hive-trunk-hadoop2 #425 (See https://builds.apache.org/job/Hive-trunk-hadoop2/425/)
HIVE-5260 : Introduce HivePassThroughOutputFormat that allows Hive to use general purpose OutputFormats instead of HiveOutputFormats in StorageHandlers (Viraj Bhat via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1522574)

• /hive/trunk/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java
• /hive/trunk/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HiveHBaseTableOutputFormat.java
• /hive/trunk/hbase-handler/src/test/results/positive/external_table_ppd.q.out
• /hive/trunk/hbase-handler/src/test/results/positive/hbase_binary_storage_queries.q.out
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HiveFileFormatUtils.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HivePassThroughOutputFormat.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/io/HivePassThroughRecordWriter.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/CreateTableDesc.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java
• /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java

Patch committed to 0.12 branch as requested.

Thejas; Thanks for the commit to branch 0.12

This issue has been fixed and released as part of 0.12 release. If you find further issues, please create a new jira and link it to this one.

Hey guys, I am struggling to understand this code. Specifically:

1) We call addHBaseResource three separate times, twice for each code path:

https://github.com/apache/hive/blob/3b96b3204e5298bcc252bf6f0f5180bb6119bff1/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java#L331
https://github.com/apache/hive/blob/3b96b3204e5298bcc252bf6f0f5180bb6119bff1/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java#L337
https://github.com/apache/hive/blob/3b96b3204e5298bcc252bf6f0f5180bb6119bff1/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java#L346

2) We copy the jobConf, call addHBaseResources a second time, and then never use the object?

https://github.com/apache/hive/blob/3b96b3204e5298bcc252bf6f0f5180bb6119bff1/hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java#L345

Any idea what is the purpose of doing these things?

Hi Brock,
Sorry for getting back to you late on this. Thanks for cleaning the code on this one.
I think during the merge of the HBaseStorageHandler, some of the code might have got copied in 2 places, before I created a function for it.
So it is redundant to call: addHBaseResources in 331 which in turn calls later calls HBaseConfiguration.addHbaseResources and also do the same while configuring both input and output job properties.
You can remove line 345. I think this is a typo.
Viraj