You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tianyi Wang (Code Review)" <ge...@cloudera.org> on 2017/12/13 02:22:49 UTC
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Tianyi Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8825
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
IMPALA-5990: End-to-end compression of metadata
Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are ~2GB for an uncompressed catalog object and ~4GB for
compressed statestore update.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.
Testing: Ran existing tests. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.
Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/client-request-state.cc
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
23 files changed, 556 insertions(+), 551 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/8825/1
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
IMPALA-5990: End-to-end compression of metadata
Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are:
- A single catalog object cannot be larger than ~2GB.
- A statestore catalog update cannot be larger than ~4GB. It is
compressed size if FLAGS_compact_catalog_topic is true.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.
Testing: Ran existing tests. A test for compressing and decompressing
catalog objects is added. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.
Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
---
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
A be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
26 files changed, 790 insertions(+), 574 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/8825/6
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 9:
Taking a look now
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 20:52:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 11: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1861/
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 06:16:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1857/
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 21:10:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
IMPALA-5990: End-to-end compression of metadata
Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are:
- A single catalog object cannot be larger than ~2GB.
- A statestore catalog update cannot be larger than ~4GB. It is
compressed size if FLAGS_compact_catalog_topic is true.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.
Testing: Ran existing tests. A test for compressing and decompressing
catalog objects is added. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.
Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
---
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
A be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
26 files changed, 817 insertions(+), 567 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/8825/10
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1863/
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 12
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 19:29:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 4:
(34 comments)
General approach and flow looks good to me.
I'm still thinking about details and testing.
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/CMakeLists.txt
File be/src/catalog/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/CMakeLists.txt@29
PS4, Line 29: ADD_BE_TEST(catalog-util-test)
I think you forgot to add the .cc file
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog-server.h
File be/src/catalog/catalog-server.h:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog-server.h@78
PS4, Line 78: void AddTopicUpdate(std::string key, const uint8_t* buf, uint32_t size, bool deleted);
* AddPendingTopicItem()
(and corresponding change to comment, see a comment elsewhere explaining our use of "topic" and "item")
* Pass the key as reference? Seems clearer what is happening. Also comment that the function takes ownership of the key memory
* 'buf' needs a better name
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog-server.cc@245
PS4, Line 245: update.topic_entries.emplace_back();
Why not this instead of the loop:
update.topic_entries = std::move(pending_topic_updates_);
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog-util.h
File be/src/catalog/catalog-util.h:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog-util.h@44
PS4, Line 44: Status CompressCatalogObject(const uint8_t* buf, uint32_t size,
Use src/dst as arg names. Same for decompress below.
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog-util.cc@145
PS4, Line 145: output->resize(static_cast<size_t>(output_buffer_len));
why static_cast?
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog-util.cc@162
PS4, Line 162: output->resize(static_cast<size_t>(decompressed_len));
why static_cast?
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/catalog/catalog.h@64
PS4, Line 64: Status GetCatalogDelta(void* server_ptr, int64_t from_version,
"CatalogServer* caller" instead of "void* server_ptr", seems clearer, you can forward declare class CatalogServer to avoid the include
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/fe-support.cc@429
PS4, Line 429: jclass caller_class, jlong callback_ctx, jbyteArray key,
Currently, we get the key bytes via String.getBytes() which I believe must create a new byte[].
If we pass the key as a jstring we can avoid a copy in most cases.
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/fe-support.cc@432
PS4, Line 432: signed char* key_buf = env->GetByteArrayElements(key, nullptr);
I think we should consider using the "critical" version of this function. See JniScopedArrayCritical from:
https://gerrit.cloudera.org/#/c/8150/6/be/src/util/jni-util.h
That patch never made it in, you could just copy the code.
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/fe-support.cc@446
PS4, Line 446: Java_org_apache_impala_service_FeSupport_NativeGetCatalogUpdate(JNIEnv* env,
NativeGetNextCatalogTopicItem() or similar
the name should convey that this is a "get next" type of call and what exactly is being iterated over
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/fe-support.cc@454
PS4, Line 454: jclass caller_class, jbyteArray hdfs_location) {
why not "jstring hdfs_location"
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/fe-support.cc@465
PS4, Line 465: jclass caller_class, jbyteArray hdfs_lib_file) {
why not "jstring hdfs_lib_file"
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/fe-support.cc@551
PS4, Line 551: (char*)"NativeAddCatalogUpdate", (char*)"(J[B[BZ)V",
make indentation consistent
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/frontend.h
File be/src/service/frontend.h:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/frontend.h@41
PS4, Line 41: /// Request to update the Impalad catalog cache. The req argument contains a vector of
Update comment on the 'req' arg
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/frontend.h@214
PS4, Line 214: // A helper class used to pass catalog object updates to the frontend.
/// comment style here and below
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/frontend.h@217
PS4, Line 217: // Return the next catalog object update. The return type is Pair<Boolean, ByteBuffer>.
Return the next catalog object from a catalog update.
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/frontend.h@220
PS4, Line 220: // return value is invalided on the next call.
Comment on error handling
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/frontend.cc
File be/src/service/frontend.cc:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/frontend.cc@274
PS4, Line 274: jclass pair_class = env->FindClass("Lorg/apache/impala/common/Pair;");
Better to get the class and method ids once during a static initialization phase. You can keep them as static members like is done in other JNI-based helpers, e.g., hbase-table.h
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/impala-server.cc@1319
PS4, Line 1319: class TopicItemIterator : public JniCatalogCacheUpdateIterator {
Let's move these and its subclasses into catalog-util.h/cc or frontend.h/cc
I could not find any guidance on this in the Google style guide. Personally, I find such inline class declarations obscure the rest of the function code and we generally don't have this pattern in the codebase.
We should also explain the motivation behind using these iterators as opposed to a simpler approach where we populate two ArrayLists in one JNI call. The benefit of the iterators is that they reduce the intermediate memory consumption, right?
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/impala-server.cc@1337
PS4, Line 1337: LOG(ERROR) << "Error decompressing catalog object: " << s.GetDetail();
I think we could deal with errors in two ways:
1. We could propagate them as exceptions in env which are picked up on the Java side. That makes it clear that the update process failed and that the catalog is in an inconsistent state.
2. Instead of returning on the first error, we could attempt to process as many objects as we can and then LOG(ERROR) a report of failed updates.
I have a slight preference for (2), but would also accept (1).
In the current code, we don't have a good way to determine whether a null return value means we have successfully process all objects or not. It's also not obvious from the log message that the entire update process is aborted.
http://gerrit.cloudera.org:8080/#/c/8825/4/be/src/service/impala-server.cc@1483
PS4, Line 1483: LOG(ERROR) << "Error serializing catalog object: " << s.GetDetail();
Same comments about error handling as above
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@395
PS4, Line 395: long callbackContext_;
Maybe call this nativeCatalogServerPtr or similar to make it clear that this is a raw pointer to a native object.
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@397
PS4, Line 397: long fromVersion_;
We only use the "_" suffix for private and protected members. I think it make sense to omit them here.
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@409
PS4, Line 409: void addTopic(TCatalogObject obj, boolean delete) throws TException {
We typically use the term "topic" in the statestore topic sense, i.e. there is a metadata topic or a cluster membership topic. A clearer function name would be addTopicItem() or addItem() or addCatalogObject() or something like that.
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@418
PS4, Line 418: byte[] data = new TSerializer(new TBinaryProtocol.Factory()).serialize(obj);
Do we need to create a new protocol and serializer each time this function is called? I think we can probably have the TSerializer as a member of this class.
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java@25
PS4, Line 25: import io.netty.buffer.ByteBuf;
remove?
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java@26
PS4, Line 26: import javolution.io.Struct;
remove?
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java@86
PS4, Line 86: // Add a topic update to the backend pending topic updates. 'serializationBuffer' is a
Adds a topic item to the backend's pending metadata-topic update.
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java@91
PS4, Line 91: // Get a topic update from the backend.
// Get a catalog topic item from the backend,
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java@93
PS4, Line 93: public native static Pair<Boolean, ByteBuffer> NativeGetCatalogUpdate(
NativeGetCatalogTopicItem()
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java@94
PS4, Line 94: long callbackContext);
callbackContext sounds mysterious, how about nativeIteratorPtr or another name that makes it clear that this is a native pointer to an object (or more specifically an iterator)
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java@96
PS4, Line 96: public native static void NativeLibCacheSetNeedsRefresh(byte[] hdfs_location);
* camel case for args here and below
* why is the location not a String (jstring in JNI)? Properly typing the arg makes the function easier to read/understand
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java@97
PS4, Line 97:
remove blank line
http://gerrit.cloudera.org:8080/#/c/8825/4/fe/src/main/java/org/apache/impala/service/FeSupport.java@98
PS4, Line 98: public native static void NativeLibCacheRemoveEntry(byte[] hdfs_lib_file);
* camel case for arg
* why is the location not a String (jstring in JNI)? Properly typing the arg makes the function easier to read/understand
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jan 2018 05:33:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
IMPALA-5990: End-to-end compression of metadata
Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are:
- A single catalog object cannot be larger than ~2GB.
- A statestore catalog update cannot be larger than ~4GB. It is
compressed size if FLAGS_compact_catalog_topic is true.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.
Testing: Ran existing tests. A test for compressing and decompressing
catalog objects is added. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.
Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Reviewed-on: http://gerrit.cloudera.org:8080/8825
Reviewed-by: Tianyi Wang <tw...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
A be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
27 files changed, 815 insertions(+), 563 deletions(-)
Approvals:
Tianyi Wang: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 13
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 9: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Jan 2018 22:48:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 8:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-server.h
File be/src/catalog/catalog-server.h:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-server.h@78
PS6, Line 78: /// The return value is true if the operation succeeds and false otherwise.
> topic_data -> item_data
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util-test.cc
File be/src/catalog/catalog-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util-test.cc@40
PS6, Line 40: uint32_t large_string_size = 0x7E000000; // LZ4_MAX_INPUT_SIZE
> * reserve() the size
- Done.
- LZ4 cannot compress 2GB data. I changed it to the exact LZ4_MAX_INPUT_SIZE now.
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h
File be/src/catalog/catalog-util.h:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@36
PS6, Line 36: public:
> space before private/protected/public
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@45
PS6, Line 45: /// will be skipped and the next valid object is returned.
> will be skipped and the next valid object is returned.
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@46
PS6, Line 46: virtual jobject next(JNIEnv* env) = 0;
> Better to add a virtual destructor
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@54
PS6, Line 54:
> nit: use common abbreviations for JNI stuff, "cl" for "class" and "ctor" fo
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.cc@106
PS6, Line 106: ++pos_;
> ++pos
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.cc@118
PS6, Line 118: }
> remove newline
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@428
PS6, Line 428: JNIEXPORT void JNICALL
> I'm thinking this function should return a bool for success/failure. That w
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@435
PS6, Line 435: if (!JniUtfCharGuard::create(env, key, &key_str).ok()) return;
> Comment seems wrong, the FE won't see an exception.
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@455
PS6, Line 455: Java_org_apache_impala_service_FeSupport_NativeLibCacheSetNeedsRefresh(JNIEnv* env,
> Same comment about returning bool.
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@468
PS6, Line 468:
> Same comment about returning bool.
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/util/jni-util.h
File be/src/util/jni-util.h:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/util/jni-util.h@172
PS6, Line 172: public:
> single space before public/private
Done
http://gerrit.cloudera.org:8080/#/c/8825/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/8825/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@404
PS6, Line 404: TSerializer serializer;
> move to prev line
The previous line has 89 characters and " {" doesn't fit.
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Jan 2018 18:52:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
IMPALA-5990: End-to-end compression of metadata
Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are:
- A single catalog object cannot be larger than ~2GB.
- A statestore catalog update cannot be larger than ~4GB. It is
compressed size if FLAGS_compact_catalog_topic is true.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.
Testing: Ran existing tests. A test for compressing and decompressing
catalog objects is added. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.
Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
---
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
A be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
27 files changed, 815 insertions(+), 563 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/8825/12
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 12
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
IMPALA-5990: End-to-end compression of metadata
Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are:
- A single catalog object cannot be larger than ~2GB.
- A statestore catalog update cannot be larger than ~4GB. It is
compressed size if FLAGS_compact_catalog_topic is true.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.
Testing: Ran existing tests. A test for compressing and decompressing
catalog objects is added. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.
Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
---
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
A be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
26 files changed, 816 insertions(+), 577 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/8825/8
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
IMPALA-5990: End-to-end compression of metadata
Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are:
- A single catalog object cannot be larger than ~2GB on openjdk 8. It
cannot be larger than ~1GB on openjdk 7.
- A statestore catalog update cannot be larger than ~4GB, compressed.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.
Testing: Ran existing tests. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.
Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
22 files changed, 576 insertions(+), 553 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/8825/2
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, Dimitris Tsirogiannis, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8825
to look at the new patch set (#9).
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
IMPALA-5990: End-to-end compression of metadata
Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are:
- A single catalog object cannot be larger than ~2GB.
- A statestore catalog update cannot be larger than ~4GB. It is
compressed size if FLAGS_compact_catalog_topic is true.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.
Testing: Ran existing tests. A test for compressing and decompressing
catalog objects is added. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.
Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
---
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
A be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
26 files changed, 817 insertions(+), 577 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/8825/9
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has abandoned this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8825/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/8825/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@431
PS8, Line 431: LOG.error("NativeAddPendingTopicItem failed in BE. key=" + key + ", delete="
> add some details about the object like key, delete and data size
Done
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Jan 2018 22:46:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 9:
(5 comments)
Great job!
http://gerrit.cloudera.org:8080/#/c/8825/9/be/src/catalog/catalog-util.h
File be/src/catalog/catalog-util.h:
http://gerrit.cloudera.org:8080/#/c/8825/9/be/src/catalog/catalog-util.h@81
PS9, Line 81: explicit
Why do you need that?
http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/CatalogInternalService.thrift
File common/thrift/CatalogInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/CatalogInternalService.thrift@30
PS9, Line 30: NativeAddTopicUpdate()
Did you mean AddPendingTopicItem? I couldn't find this function.
http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/CatalogInternalService.thrift@41
PS9, Line 41: / List of updated (new and modified) catalog objects whose catalog verion is
: // larger than TGetCatalotDeltaRequest.from_version. Deprecated after IMPALA-5990.
: 2: optional list<CatalogObjects.TCatalogObject> updated_objects_deprecated
:
: // List of deleted catalog objects whose catalog version is larger than
: // TGetCatalogDelta.from_version. Deprecated after IMPALA-5990.
: 3: optional list<CatalogObjects.TCatalogObject> deleted_objects_deprecated
Why keeping them around?
http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/Frontend.thrift@668
PS9, Line 668: // New or modified items. Empty list if no items were updated. Deprecated after
: // IMPALA-5990.
: 3: optional list<CatalogObjects.TCatalogObject> updated_objects_deprecated
:
: // Empty if no items were removed or is_delta is false. Deprecated after IMPALA-5990.
: 4: optional list<CatalogObjects.TCatalogObject> removed_objects_deprecated
Same here. Do we need to keep them around?
http://gerrit.cloudera.org:8080/#/c/8825/9/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java:
http://gerrit.cloudera.org:8080/#/c/8825/9/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@29
PS9, Line 29: import org.apache.impala.thrift.*;
We typically avoid the wildcard import. thrift.* has lot's of stuff you currently don't need.
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jan 2018 22:46:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Feb 2018 20:13:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1861/
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 02:37:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 8: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8825/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/8825/8/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@431
PS8, Line 431: LOG.error("NativeAddPendingTopicItem failed in BE.");
add some details about the object like key, delete and data size
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Jan 2018 22:09:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 12: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 12
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:17:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 3:
(5 comments)
Thanks!
http://gerrit.cloudera.org:8080/#/c/8825/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8825/2//COMMIT_MSG@17
PS2, Line 17: - A single catalog object cannot be larger than ~2GB.
> It turns out the 1GB limit is fixed in some jdk7 release. openjdk7 has the
Ack
http://gerrit.cloudera.org:8080/#/c/8825/2//COMMIT_MSG@23
PS2, Line 23: Testing: Ran existing tests. A test for compressing and decompressing
> To produce it I modified the code and manually added two 1.95GB objects. It
Probably not, but it may be useful to have around, depending on how you did it. Up to you.
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-server.cc@384
PS2, Line 384: VLOG(1) << "Publishing " << (deleted ? "deletion: " : "update: ") << item.key <<
> Added size into the logging. The backend doesn't know the version so the ve
Ack
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-util.cc@139
PS2, Line 139: Status CompressCatalogObject(const uint8_t* buf, uint32_t size, string* output) {
> We can change DecompressCatalogObject() to using string. I'm not sure if we
Ok.
http://gerrit.cloudera.org:8080/#/c/8825/2/fe/src/main/java/org/apache/impala/util/TByteBuffer.java
File fe/src/main/java/org/apache/impala/util/TByteBuffer.java:
http://gerrit.cloudera.org:8080/#/c/8825/2/fe/src/main/java/org/apache/impala/util/TByteBuffer.java@26
PS2, Line 26: * ByteBuffer-backed implementation of TTransport.
> I copied it from thrift and removed the unused functions. I'm not sure what
It's definitely ok to copy, but I'm not sure if need to do more.
At the very least, let's make sure to indicate here that it's a copy, so when we upgrade Thrift, we can rip this out.
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Dec 2017 19:57:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
IMPALA-5990: End-to-end compression of metadata
Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are:
- A single catalog object cannot be larger than ~2GB.
- A statestore catalog update cannot be larger than ~4GB. It is
compressed size if FLAGS_compact_catalog_topic is true.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.
Testing: Ran existing tests. A test for compressing and decompressing
catalog objects is added. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.
Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
---
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
A be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impalad-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
26 files changed, 811 insertions(+), 559 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/8825/11
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 2:
(10 comments)
Hi,
I've not done a deep review of the protocol changes, but took a look through to understand new things for myself.
I think it would be useful to write the boring test for the C++ CompressCatalogObject() and DecompressCatalogObject() functions. We could check that things like the empty string go through this just fine. (I have no reason to believe they wouldn't.)
We should also decide on our approach to Thrift structs. My biases are to keep them compatible (i.e., don't re-use numbers, keep around deprecated fields forever), but we should decide explicitly, and perhaps update the style guide.
http://gerrit.cloudera.org:8080/#/c/8825/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8825/2//COMMIT_MSG@17
PS2, Line 17: - A single catalog object cannot be larger than ~2GB on openjdk 8. It
This is probably just "jdk 8" (rather than Open). A lot of people run Impala using the Oracle JDK, so it'd be good to know that Oracle and OpenJDK agree here.
http://gerrit.cloudera.org:8080/#/c/8825/2//COMMIT_MSG@23
PS2, Line 23: Testing: Ran existing tests. Manually tested with a 1.95GB catalog object
Was this painful to produce? Could we check in a test that creates a huge object and tries to force this to happen, or is it prohibitively expensive?
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-server.cc@384
PS2, Line 384: VLOG(1) << "Publishing " << (deleted ? "deletion " : "update ") << ": " << item.key;
Would logging size (perhaps both compressed and uncompressed) here be useful? The old log also had a catalog version; I think that's useful to keep.
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-util.h
File be/src/catalog/catalog-util.h:
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-util.h@41
PS2, Line 41: /// 'catalog_object'. Stores the size of the uncopressed catalog object in the first
spelling: uncompressed
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/8825/2/be/src/catalog/catalog-util.cc@139
PS2, Line 139: Status CompressCatalogObject(const uint8_t* buf, uint32_t size, string* output) {
Does the asymmetric of using string* for output here, and vector<uint8_t>* output in decompression bother us?
http://gerrit.cloudera.org:8080/#/c/8825/2/common/thrift/CatalogInternalService.thrift
File common/thrift/CatalogInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/8825/2/common/thrift/CatalogInternalService.thrift@28
PS2, Line 28: 1: required i64 callback_ctx
I think this should be "2", to preserve the ordering. We don't explicitly support multiple versions of these things working together, but it's good to honor Thrift's goals here.
http://gerrit.cloudera.org:8080/#/c/8825/2/common/thrift/CatalogInternalService.thrift@39
PS2, Line 39: 1: required i64 max_catalog_version
Technically we should leave 2 and 3 around as optional, renaming them to _deprecated or some such.
http://gerrit.cloudera.org:8080/#/c/8825/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/8825/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@24
PS2, Line 24: import java.util.*;
I think we should discourage wildcard imports. (You can configure your IDE to not do this.)
We don't formally adopt Google's Java style guide, but https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports is the reference. It's especially bad in line 46, where nobody knows what might be in org.apache.impala.common.*.
http://gerrit.cloudera.org:8080/#/c/8825/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java:
http://gerrit.cloudera.org:8080/#/c/8825/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@162
PS2, Line 162: LOG.trace((update.first ? "Deleting " : "Adding ") + "item: " + key + " version: "
I don't know if this is performance sensitive, but slf4j has some mechanism to say:
if (LOG.isEnabled(TRACE)) { ... }
That would avoiding doing the string interpolation if it's not going to just throw it away.
(I didn't look up the syntax for slf4j here.)
http://gerrit.cloudera.org:8080/#/c/8825/2/fe/src/main/java/org/apache/impala/util/TByteBuffer.java
File fe/src/main/java/org/apache/impala/util/TByteBuffer.java:
http://gerrit.cloudera.org:8080/#/c/8825/2/fe/src/main/java/org/apache/impala/util/TByteBuffer.java@26
PS2, Line 26: * ByteBuffer-backed implementation of TTransport.
https://github.com/apache/thrift/blame/master/lib/java/src/org/apache/thrift/transport/TByteBuffer.java
https://issues.apache.org/jira/browse/THRIFT-3432
Looks like Thrift has added this. We've got other work to upgrade to a more recent version.
The implementations are slightly different; let's make sure ours makes sense?
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2017 21:52:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 10: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1857/
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 00:44:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has restored this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Restored
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: restore
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 6:
(14 comments)
I'm pretty happy with this change, probably good to get another pair of eyes. Let's ask Dimitris to review.
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-server.h
File be/src/catalog/catalog-server.h:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-server.h@78
PS6, Line 78: void AddPendingTopicItem(std::string key, const uint8_t* topic_data, uint32_t size,
topic_data -> item_data
(the topic is a container of items, and we are passing the data of a single item)
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util-test.cc
File be/src/catalog/catalog-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util-test.cc@40
PS6, Line 40: for (int i = 0; i < 2000 * 1024 * 1024; ++i) { // almost 2GB
* reserve() the size
* why almost 2GB? is there any significance to 2GB? add comment
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h
File be/src/catalog/catalog-util.h:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@36
PS6, Line 36: public:
space before private/protected/public
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@45
PS6, Line 45: /// will be skipped and the catalog update will still proceed.
will be skipped and the next valid object is returned.
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@46
PS6, Line 46: virtual jobject next(JNIEnv* env) = 0;
Better to add a virtual destructor
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.h@54
PS6, Line 54: static jclass pair_class;
nit: use common abbreviations for JNI stuff, "cl" for "class" and "ctor" for "constructor", i.e.:
pair_cl, pair_ctor, boolean_cl, boolean_ctor
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.cc@106
PS6, Line 106: pos_ += 1;
++pos
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/catalog/catalog-util.cc@118
PS6, Line 118:
remove newline
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@428
PS6, Line 428: JNIEXPORT void JNICALL
I'm thinking this function should return a bool for success/failure. That way the caller can decide whether what to do with this issue. I think we should probably log something and move on in most cases.
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@435
PS6, Line 435: // Throw any exception into the frontend
Comment seems wrong, the FE won't see an exception.
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@455
PS6, Line 455: JNIEXPORT void JNICALL
Same comment about returning bool.
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/service/fe-support.cc@468
PS6, Line 468: JNIEXPORT void JNICALL
Same comment about returning bool.
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/util/jni-util.h
File be/src/util/jni-util.h:
http://gerrit.cloudera.org:8080/#/c/8825/6/be/src/util/jni-util.h@172
PS6, Line 172: public:
single space before public/private
http://gerrit.cloudera.org:8080/#/c/8825/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/8825/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@404
PS6, Line 404: {
move to prev line
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jan 2018 21:56:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata
......................................................................
Patch Set 3:
Can you see my responses in the comments? I deleted a draft patchset and as a result I can only see my comments with my account logged out now.
--
To view, visit http://gerrit.cloudera.org:8080/8825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Dec 2017 19:38:24 +0000
Gerrit-HasComments: No