Henry Robinson has posted comments on this change. Change subject: IMPALA-3499: Split catalog update ......................................................................
Patch Set 9: (4 comments) This looks a lot better to me. Beyond the exhaustive test, how else have you tested this? http://gerrit.cloudera.org:8080/#/c/3067/9/be/src/service/frontend.h File be/src/service/frontend.h: PS9, Line 38: The TUpdateCatalogCacheRequest contains : /// a list of objects that should be added/removed from the Catalog. Returns a response : /// that contains details such as the new max catalog version. Update the comment http://gerrit.cloudera.org:8080/#/c/3067/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS5, Line 1354: ImpaladMetrics::CATALOG_READY->set_value(new_catalog_version > 0); : UpdateCatalogMetrics(); : > I am not sure how... because when at line 1354, the code does not know if a I mean not having any logic regarding taking the lock, and just unconditionally acquiring it. http://gerrit.cloudera.org:8080/#/c/3067/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS9, Line 1278: LOG(WARNING) This should be VLOG(1) or 2. http://gerrit.cloudera.org:8080/#/c/3067/9/be/src/util/jni-util.h File be/src/util/jni-util.h: PS9, Line 286: jbytearray_2D Do you know if this copies the bytes? If so, this might be a bit expensive in terms of memory (a 2GB update now takes 4GB of RAM). How about using DirectByteBuffers in the loop? So you can do: jobject bb = (*env)->NewDirectByteBuffer(env, (void*) &args[i], sizeof(MyNativeStruct)); jni_env->SetObjectArrayElement(jbytearray_2D, i, bb) As I understand it, that shouldn't copy the bytes between Java and C++. -- To view, visit http://gerrit.cloudera.org:8080/3067 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I176db25124a32944f2396ce8aafbed49cac95928 Gerrit-PatchSet: 9 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-HasComments: Yes
