Joe McDonnell has posted comments on this change. Change subject: IMPALA-4687: Get Impala working against HBase 2.0 ......................................................................
Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/7277/1//COMMIT_MSG Commit Message: PS1, Line 25: This was tested by verifying that an HBase 2.0 cluster : starts up. > I think we should run hbase stress test eventually to test the new ScannerT I think we can run stress tests with existing code. Since HBase stopped throwing this exception with HBASE-16266, I think this is already available in versions of HBase 1.2, which we already support. http://gerrit.cloudera.org:8080/#/c/7277/1/be/src/exec/hbase-table-scanner.cc File be/src/exec/hbase-table-scanner.cc: PS1, Line 238: : > May be retain this part. Done PS1, Line 197: setStartRow > You mentioned on the jira that these are deprecated too (setStartRow/setSto It turns out that setStartRow/setStopRow are newly deprecated. The new methods (with*Row) do not exist in HBase 1.2, so we can't replace it without having conditional code to detect the different APIs. I don't think it is worth switching yet. I will file a separate JIRA. PS1, Line 403: jthrowable exc = env->ExceptionOccurred(); : if (exc == NULL) return Status::OK(); > Unrelated to your change, but why are we calling this here? Shouldn't GetJn We need the actual jthrowable so that we can check whether it is an instance of ScannerTimeoutException. GetJniExceptionMsg only returns the message. Line 407: // necessary). For HBase 2.0, ScannerTimeoutException does not exist, so simple > typo: simply Done PS1, Line 407: simple > simply? Done Line 411: if (scanner_timeout_ex_cl_ == nullptr) return status; > Is there new exception that signals we need to retry? Is the retry not nece >From what I understand, HBase does a heartbeat now, and where it previously >would throw this exception, it now resets the scanner and retries. HBASE-16266 >is when this changed. Line 512: // Newer versions of HBase re-create the ResultScanner without throwing this > Is "re-create" the right term? I'm not sure how that would work. Do you mea Reworked this comment. Line 514: // code will simply return the error. > what is "the error"? Fixed this language. http://gerrit.cloudera.org:8080/#/c/7277/1/be/src/exec/hbase-table-scanner.h File be/src/exec/hbase-table-scanner.h: Line 61: /// 1. Scan.setCaching and Scan.setCacheBlocks tolerate the older void return value > use () for function/methods, e.g. Scan.setCaching() Done http://gerrit.cloudera.org:8080/#/c/7277/1/be/src/exec/hbase-table-writer.h File be/src/exec/hbase-table-writer.h: PS1, Line 108: add > addColumn Done http://gerrit.cloudera.org:8080/#/c/7277/1/be/src/util/jni-util.cc File be/src/util/jni-util.cc: Line 61: env->GetMethodID(class_ref, method_str, method_signature); > Reading the documentation for "GetMethodID" Good point. I haven't looked at the return value, but it does throw NoSuchMethodException. Maybe it also returns null. I think either would work, but in either case, we would need to clear the exception. http://gerrit.cloudera.org:8080/#/c/7277/1/be/src/util/jni-util.h File be/src/util/jni-util.h: PS1, Line 171: a method > ..non static method.. Done -- To view, visit http://gerrit.cloudera.org:8080/7277 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I87610e25c01b3547ec332c6975b61284b6837d27 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-HasComments: Yes
