Can you please create a reviewboard link ? Can you also add the config parameters to hive-default.xml.template and give a description there ? Indentation seems to be off at some places, can you also please fix that ?

1) There are many tabs and spacing issues in this patch
2) We are catching throwable and logging at debug. We should never do this.

} catch (Throwable t) {
LOG.debug("CBO failed, skipping CBO", t);

@Brock #2 Is the concern that we are swallowing the exception or is it that the log level should be info or warning instead of debug?

Review at https://reviews.apache.org/r/18172/

Overall: +1 all checks pass

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

SUCCESS: +1 5120 tests passed

Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1348/testReport
Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1348/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.

ATTACHMENT ID: 12629280

Hi Laljo,

Essentially that is correct. The problem with that code is that it catches Throwable and logs at debug. This means nasty errors such as OOM or internal JVM errors which occur and will not be logged since most users do not log at the debug level in production. I'd suggest not catching throwable and instead catching exception. Catching exception is still a poor coding practice as you will catch all kinds of runtime errors which you do not expect. I am guessing the author is looking to "try CBO" and fail back if a bug is hit. Even in that case, catching exception, we should be logging at a ERROR level unless there is a very good reason not to.

Overall: -1 at least one tests failed

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

ERROR: -1 due to 1 failed/errored test(s), 5127 tests executed
Failed tests:

org.apache.hadoop.hive.cli.TestMinimrCliDriver.testCliDriver_bucketmapjoin6

Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1362/testReport
Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1362/console

Messages:

Executing org.apache.hive.ptest.execution.PrepPhase
Executing org.apache.hive.ptest.execution.ExecutionPhase
Executing org.apache.hive.ptest.execution.ReportingPhase
Tests exited with: TestsFailedException: 1 tests failed

This message is automatically generated.

ATTACHMENT ID: 12629326

AS requested by John:

• raise default for 'hive.cbo.max.joins.supported' to 10
• fix name in hive-default.xml

Overall: -1 no tests executed

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

Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1379/testReport
Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1379/console

Messages:

Executing org.apache.hive.ptest.execution.PrepPhase
Tests exited 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 '
+ ANT_OPTS='-Xmx1g -XX:MaxPermSize=256m '
+ export 'M2_OPTS=-Xmx1g -XX:MaxPermSize=256m -Dhttp.proxyHost=localhost -Dhttp.proxyPort=3128'
+ M2_OPTS='-Xmx1g -XX:MaxPermSize=256m -Dhttp.proxyHost=localhost -Dhttp.proxyPort=3128'
+ cd /data/hive-ptest/working/
+ tee /data/hive-ptest/logs/PreCommit-HIVE-Build-1379/source-prep.txt
+ [[ false == \t\r\u\e ]]
+ 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 .
++ awk '{print $2}'
++ egrep -v '^X|^Performing status on external'
++ svn status --no-ignore
+ rm -rf
+ svn update

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

At revision 1569331.
+ 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, p1, or p2
+ exit 1
'

This message is automatically generated.

ATTACHMENT ID: 12629471

revert to .5 patch as HIVE-6037 has been reverted.

+1 LGTM

Is plan to get CBO optimizer work in 0.13 timeframe? If no, I think we may want to delay this till branching 0.13 Otherwise, this will go in 0.13 release with a config which doesn't do anything and thus will be confusing for end users.

yes agreed, let's add this post hive 0.13 branching.

This has been invalidated by the creation of the cbo branch.