date:20170907

[Impala-ASF-CR] IMPALA-5538: Use explicit catalog versions for deleted objects

2017-09-07 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has uploaded a new patch set (#2).

Change subject: IMPALA-5538: Use explicit catalog versions for deleted objects
..

IMPALA-5538: Use explicit catalog versions for deleted objects

This commit changes the way deletions are handled in the catalog and
disseminated to the impalad nodes through the statestore. Previously,
deletions of catalog objects were not explicitly annotated with the
catalog version in which the deletion occured and the impalads were
using the max catalog version in a catalog update in order to decide
whether a deletion should be applied to the local catalog cache or not.
This works correctly under the assumption that
all the changes that occurred in the catalog between an update's min
and max catalog version are included in that update, i.e. no gaps or
missing updates. With the upcoming fix for IMPALA-5058, that constraint
will be relaxed, thus allowing for gaps in the catalog updates.

To avoid breaking the existing behavior, this patch introduced the
following changes:
* Deletions in the catalog are explicitly recorded in a log with
the catalog version in which they occurred. As before, added and deleted
catalog objects are sent to the statestore.
* Topic entries associated with deleted catalog objects have non-empty
values (besided keys) that contain minimal object metadata including the
catalog version.
* Statestore is no longer using the existence or not of
topic entry values in order to identify deleted topic entries. Deleted
topic entries should be explicitly marked as such by the statestore
subscribers that produce them.
* Statestore subscribers now use the 'deleted' flag to determine if a
topic entry corresponds to a deleted item.
* Impalads use the deleted objects' catalog versions when updating the
local catalog cache from a catalog update and not the update's maximum
catalog version.

Testing:
- No new tests were added as these paths are already exercised by
existing tests.
- Run all core tests.

Change-Id: I93cb7a033dc8f0d3e0339394b36affe14523274c
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.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/CatalogOpExecutor.java
M tests/statestore/test_statestore.py
16 files changed, 438 insertions(+), 331 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/7731/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7731
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I93cb7a033dc8f0d3e0339394b36affe14523274c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis

[Impala-ASF-CR] [PREVIEW] IMPALA-5538: Use explicit catalog versions for deleted objects

2017-09-07 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change.

Change subject: [PREVIEW] IMPALA-5538: Use explicit catalog versions for 
deleted objects
..


Patch Set 1:

(28 comments)

http://gerrit.cloudera.org:8080/#/c/7731/1/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

Line 288: BuildTopicUpdates(catalog_objects.updated_objects, false);
> Is there a requirement/assumption that the deletions must come after the up
No, it doesn't matter.


Line 300: void CatalogServer::BuildTopicUpdates(const vector& 
catalog_objects,
> IMO, it is more readable if the signature of BuildTopicUpdates takes TGetAl
These two "do something" blocks are almost identical. I refactored the code a 
bit to make it a bit less flaky.


Line 333: << catalog_object.catalog_version;
> indent
Done


http://gerrit.cloudera.org:8080/#/c/7731/1/be/src/catalog/catalog-server.h
File be/src/catalog/catalog-server.h:

Line 154:   /// determine items that have been deleted, it saves the set of 
topic entry keys sent
> Update comment
Done


Line 164:   bool topic_deletions);
> What does this param do?
Updated the comment.


http://gerrit.cloudera.org:8080/#/c/7731/1/be/src/scheduling/scheduler-test-util.cc
File be/src/scheduling/scheduler-test-util.cc:

Line 481:   item.key = host.ip;
> Use __set fn like above to be consistent
Done


http://gerrit.cloudera.org:8080/#/c/7731/1/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

Line 141:   if (delta.is_delta && delta.topic_entries.empty()) {
> single line
Done


http://gerrit.cloudera.org:8080/#/c/7731/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 1323:   TCatalogObject catalog_object;
> Please move this right before DeserializeThriftMsg() so we can see where it
Done


Line 1343:  << item.key << " is "
> indentation
Done


Line 1367: LOG(INFO) << "Deleted item: " << item.key << " version: " << 
catalog_object.catalog_version;
> That's a lot of logging, sure we need this at INFO level?
Sorry, that was for debugging.


Line 1398: if (existing_object.catalog_version <= 
catalog_object.catalog_version) {
> Shouldn't this be < and not <=?
Done


PS1, Line 1406: batch_size_bytes
> Looks like we didn't account this earlier for deletions. This logic makes m
Done


Line 1527:   if (item.deleted) {
> Is it possible that within a single list of topic_entries there is both an 
No, it's not possible to have both an addition and a deletion in the same list 
of topic_entries.


http://gerrit.cloudera.org:8080/#/c/7731/1/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

Line 485: item.value.empty() ? Statestore::TopicEntry::NULL_VALUE : 
item.value,
> Why not just 'item.value'? The value gets copied.
Good point. Done


Line 527:   deleted_key_size_bytes -= itr->first.size();
> += ?
Done


http://gerrit.cloudera.org:8080/#/c/7731/1/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:

Line 92
> - I kind of like this separation of updates and deletions in different list
By switching to TTopicItem for topic deletions there is now significant overlap 
between operations performed on both added and deleted items. We could try to 
extract most of the common logic in separate functions and keep the existing 
logic in place, but for now it seems simple enough to have one loop over one 
list of items and separate the logic with an if statement when needed. If we 
had to add many of these if statements in multiple places, I'd agree that 
splitting the logic would have been better.


Line 97:   5: required bool is_delta;
> fix member numbering
Done


http://gerrit.cloudera.org:8080/#/c/7731/1/fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java:

Line 56:   private Map removedCatalogObjectVersionsByKey_ = 
Maps.newHashMap();
> I might be wrong, but I think we can end up having the same object deleted 
Good catch, that was a bug indeed. Removed the second map and simplified the 
logic around handling the deleted objects.


Line 70:* key 'key'.
> update comment
No longer applicable. Function is removed.


Line 90:   public synchronized void garbageCollect(long currentCatalogVersion) {
> Although these methods are synchronized, the underlying maps aren't thread 
That's not correct. The essence of having synchronized functions is exactly 
that, to ensure proper synchronization even though the underlying collections 
are not thread safe. Two threads calling garbageCollect and addRemoved cannot 
both proceed, one will grab the lock on the CatalogDeltaLog object and make 
progress while the other will simply block. In any case, this class is 
simplified and it only contains one collection.


Line 154:   private String toCatalogObjectKey(TCatalogObject catalogObject) {

[Impala-ASF-CR] IMPALA-5881: Use native allocation while building catalog updates

2017-09-07 Thread Alex Behm (Code Review)

Alex Behm has posted comments on this change.

Change subject: IMPALA-5881: Use native allocation while building catalog
updates
..

Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7955/10/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

Line 151: if (serializedByteSize >=
SERIALIZED_TOPIC_SIZE_WARNING_LIMIT_BYTES) {
Dimitris and I discussed this warning. If we decide to issue a warning, then I
think we should warn when close to the limit, e.g. at 3GB. The warning message
should pretty-print the number of bytes and also print the hard limit. Also the
message should be more assertive, e.g. "potentially" is vague and should be
removed (we know the exact serialized size).

For inspiration, let me propose this msg:

Serialized size of catalog update approaching hard limit. Current size: 3GB.
Hard limit: 4GB. Catalog updates that exceed the limit will be ignored and will
result in stale metadata on coordinator nodes.

--
To view, visit http://gerrit.cloudera.org:8080/7955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I383684effa9524734ce3c6c0fb7ed37de0e15782
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Dimitris Tsirogiannis
Gerrit-Reviewer: Mostafa Mokhtar
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes

50 matches

Mail list logo