You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org> on 2017/11/14 17:54:57 UTC
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Dimitris Tsirogiannis has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8545
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
[PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Problem: A long running table metadata operation (e.g. refresh) could
prevent any other metadata operation from making progress if it
coincided with the gather catalog updates operation. The problem was due
to the conservative locking scheme used when catalog updates were
collected. In particular, in order to collect a consistent snapshot of
metadata changes, the global catalog lock was held for the entire
duration of that operation.
Solution: To improve the concurrency of catalog operations the following
changes are performed:
* A range of catalog versions determines the catalog changes to be
included in a catalog update. Any catalog changes that do not fall in
the specified range are ignored (to be processed in subsequent catalog
updates).
* The catalog allows metadata operations to make progress while collecting
catalog updates.
* To prevent starvation of catalog updates (i.e. frequently updated
catalog objects skipping catalog updates indefinitely), we keep track
of the number of times a catalog object has skipped an update and if
that number exceeds a threshold it is included in the next catalog
update even if its version is not in the specified catalog update
version range. Hence, the same catalog object may be sent in two
consecutive catalog updates.
Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
---
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/CatalogService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java
A fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
A fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/DataSource.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Role.java
M fe/src/main/java/org/apache/impala/catalog/RolePrivilege.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.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/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
30 files changed, 1,153 insertions(+), 458 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/8545/1
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8545
to look at the new patch set (#2).
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
[PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Problem: A long running table metadata operation (e.g. refresh) could
prevent any other metadata operation from making progress if it
coincided with the gather catalog updates operation. The problem was due
to the conservative locking scheme used when catalog updates were
collected. In particular, in order to collect a consistent snapshot of
metadata changes, the global catalog lock was held for the entire
duration of that operation.
Solution: To improve the concurrency of catalog operations the following
changes are performed:
* A range of catalog versions determines the catalog changes to be
included in a catalog update. Any catalog changes that do not fall in
the specified range are ignored (to be processed in subsequent catalog
updates).
* The catalog allows metadata operations to make progress while collecting
catalog updates.
* To prevent starvation of catalog updates (i.e. frequently updated
catalog objects skipping catalog updates indefinitely), we keep track
of the number of times a catalog object has skipped an update and if
that number exceeds a threshold it is included in the next catalog
update even if its version is not in the specified catalog update
version range. Hence, the same catalog object may be sent in two
consecutive catalog updates.
Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
---
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/CatalogService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java
A fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
A fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/DataSource.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Role.java
M fe/src/main/java/org/apache/impala/catalog/RolePrivilege.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
29 files changed, 1,321 insertions(+), 534 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/8545/2
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 2
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@596
PS1, Line 596: addTableToCatalogDeltaHelper(tbl, fromVersion, toVersion, resp);
> I see, thanks for the explanation. I thought we are trying to make getCatal
Makes sense. We thought making getCatalogDelta() completely non-blocking, but it's really really hard so we decided against it for now. Think about it this way, you could have a table that is being changed all the time, so if you never decide wait for the table lock, then you might never get an update. Eventually you have to bite the bullet and wait.
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 07:22:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@596
PS1, Line 596: addTableToCatalogDeltaHelper(tbl, fromVersion, toVersion, resp);
> Maybe I'm misunderstanding something but I still see the lock contention on
I think your assessment is correct.
We are not trying to make getCatalogDelta() completely non-blocking. As you've correctly pointed out it can still block on a table lock.
The problem we are trying to solve is that non-conflicting operations should not conflict. Before this change the global lock was held for the entire duration of getCatalogDelta(). If a long-running operation is running against tbl A, and the thread executing getCatalogDelta() needs to wait to get the table lock on A, then no concurrent DDL whatsoever is possible due to the global lock. That's the main problem: a long running operation on A can prevent concurrent DDL against anything else.
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 06:42:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 1:
(1 comment)
Doing another full round now
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@606
PS1, Line 606: new CatalogUpdateLogEntry(updateEntry.getNumSkippedCatalogUpdates() + 1,
> Actually, the reason I am doing it like that is to avoid the need for synch
Synchronize between what? There's only one thread calling getCatalogDelta() and the background GC thread does not modify the TopicUpdateLogEntries.
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Nov 2017 22:29:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 2:
(22 comments)
The renames make the code a lot easier to read, thank you!
http://gerrit.cloudera.org:8080/#/c/8545/2/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/8545/2/be/src/service/client-request-state.cc@187
PS2, Line 187: reset_req.reset_metadata_params.__set_sync_ddl(
Why do we need to set sync_ddl twice?
http://gerrit.cloudera.org:8080/#/c/8545/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8545/2/be/src/service/impala-server.cc@1430
PS2, Line 1430: LOG(INFO) << "Update applied with version: " << new_catalog_version
Catalog update applied...
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java:
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java@29
PS2, Line 29: public class CatalogObjectVersionManager {
Name is not very telling. Some alternatives:
CatalogObjectVersionQueue
CatalogObjectVersions
ActiveCatalogObjectVersions
CurrentCatalogObjectVersions
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java@37
PS2, Line 37: public void updateCatalogObjectVersions(long oldVersion, long newVersion) {
Do you think it's worth checking the return codes of the calls to objectVersions_ to make sure we are in a sane state?
For example, I assume that in this call we expect 'oldVersion' to exist. Is it ok to add newVersion if the old version does not exist?
Do we expect add/remove to actually add/remove something? Does it matter?
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@606
PS1, Line 606: * Get a snapshot view of all the functions in a database.
> There is one writer (thread doing getCatalogDelta) and multiple readers (th
Ah right, got it.
http://gerrit.cloudera.org:8080/#/c/8545/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/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@86
PS2, Line 86: * and processing of DDL requests. For each DDL request, the CatalogServiceCatalog
This description of the versioning scheme is not accurate anymore.
I think the versioning, topic update, and skipping mechanics deserve to be described in more detail here.
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@96
PS2, Line 96: * - Delete log. The delete log records catalog objects that have been removed from the
Let's give an intuition for the purpose of this log, e.g.:
Since deleted objects are removed from the cache, the cache itself is not useful for tracking deletions. This log is used for populating the list of deleted objects during a topic update.
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@97
PS2, Line 97: * catalog. An entry is added to this log every time an object is removed (e.g.
An entry with a new version is added ...
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@99
PS2, Line 99: * be perfomed atomically. An entry is removed from this log from the topic update
removed from this log by the topic update thread
(also typo: performed)
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@102
PS2, Line 102: * been processed by the topic update thread. The topic update thread is the only one
that have been included in a topic update.
(The current sentence makes it sound like there is one single topic update thread, but really it's probably a different thread every time).
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@105
PS2, Line 105: * Information per catalog object includes the number of times a catalog object has
Each entry includes the number of times a catalog object has been omitted in a topic update ...
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@111
PS2, Line 111: * version that the issueing impalad must wait for in order to ensure that the effects
typo: issuing
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@112
PS2, Line 112: * of this operation have been broadcast to all the coordinators. The time-based cleanup
Why did we chose a time-based cleanup then? I think the time-based cleanup needs to be motivated if this serious-sounding issue is a consequence of that choice.
Would also be good to mention that the value of TOPIC_UPDATE_LOG_GC_FREQUENC was chosen to make the hang/timeout case unlikely.
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@129
PS2, Line 129: * that should be employed if both the catalog lock and table locks need to be held at
catalog lock -> version lock?
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@183
PS2, Line 183: // Version of the last topic update sent to the statestore.
sent -> returned (to make the control flow clearer)
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@192
PS2, Line 192: private final static int TOPIC_UPDATE_LOG_GC_FREQUENCY = 50;
Mention how this affects the lifetime of an individual entry. My understanding is that an entry can be live for at most (2*TOPIC_UPDATE_LOG_GC_FREQUENCY)-1 topic updates assuming it is not being updated
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@195
PS2, Line 195: p
Let's create a TopicUpdateLog class that encapsulates these counters and the GC logic.
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@258
PS2, Line 258: private final Object topicUpdateEventNotifier_ = new Object();
Maybe we should use the topicUpdateLog_ for this purpose once we have a separate TopicUpdateLog class. Seems a little more direct to wait/notify on the log object since changes in the log that trigger a notify. Alternatively, we can add wait/notify methods to the TopicUpdateLog itself to simplify the code in this file.
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1228
PS2, Line 1228: private void removeDbHelper(Db db) {
updateDeleteLog(Db db)?
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1938
PS2, Line 1938: topicUpdateLog_.get(CatalogDeltaLog.toCatalogObjectKey(tCatalogObject));
Move toCatalogObjectKey() to Catalog, referring to the delta log here is a little confusing
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1976
PS2, Line 1976: if (topicUpdateLog_.remove(entry.getKey(), entry.getValue())) {
Are uou sure it's ok to remove() while iterating over the map? Might be clearer to use an iterator.
http://gerrit.cloudera.org:8080/#/c/8545/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/8545/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@179
PS2, Line 179: // functions and priviles) and then the top-level objects (databases and roles).
typo: privlieges
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 2
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Nov 2017 23:39:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@596
PS1, Line 596: addTableToCatalogDeltaHelper(tbl, fromVersion, toVersion, resp);
Maybe I'm misunderstanding something but I still see the lock contention on 'tbl' here.
With the patch, given we don't lock the tables while snapshotting, there could be inflight DDLs on tbl stuck in tryLockTable() (say CatalogOpEx.alterTable()) and that still holds the tbl.lock_ and still hasn't updated tbl to a newer catalog version resulting in the if check in L595 to pass. Eventually, that blocks on L622 stalling the getCatalogDelta() thread.
For example, we are trying to snapshot objects in (90, 100] and tbl's version is 95. We have an alterTable() running on this particular table which got the new catalog version (say 105), but hasn't still set it for the 'tbl' since it is waiting for the alter() to finish. All this while, alter thread still holds the tbl.lock_, but the condition in L595 still passes with this thread blocking on L622.
(My argument is based on my understanding is that 'tbl' still refers to the same table object even after ImmutableList.copy() in L564)
Did I get something wrong?
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 06:11:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@606
PS1, Line 606: new CatalogUpdateLogEntry(updateEntry.getNumSkippedCatalogUpdates() + 1,
> Synchronize between what? There's only one thread calling getCatalogDelta()
There is one writer (thread doing getCatalogDelta) and multiple readers (threads doing metadata ops with SYNC_DDL).
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Nov 2017 00:34:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 1:
(37 comments)
Addressed comments and fixed number of races. Run core and exhaustive tests.
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.h@336
PS1, Line 336: /// Wait until the last applied catalog update has been broadcasted to the
> broadcast
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.h@337
PS1, Line 337: /// entire cluster or until the catalog service id has changed.
> entire cluster -> all coordinators
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc@1462
PS1, Line 1462: // TODO: What about query cancellation?
> Unfortunately, most DDL operations are not cancellable due to IMPALA-2568 s
Yeah, that was an old TODO. I'll remove it.
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc@1503
PS1, Line 1503: const TUniqueId& catalog_service_id = catalog_update_result.catalog_service_id;
> Document the waiting process for SYNC_DDL dance in the function comment of
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc@1507
PS1, Line 1507: // 'catalog_update_result' to determined when the effects of this operation
> to determine
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc@1512
PS1, Line 1512: WaitForCatalogUpdate(catalog_update_result.version, catalog_service_id);
> What operations go down this path?
drop db/table if exists. Let me know if a comment is needed here.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@89
PS1, Line 89: CatalogObjectVersionManager.INSTANCE.removeCatalogObjectVersion(
> What's this new code for?
It is for maintaining all the catalog object versions of the local catalog cache up to date as the catalog is updated. This is for the sole purpose of being able to answer when the minimum version in the local cache is greater than some specified catalog update version (see INVALIDATE METADATA).
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@20
PS1, Line 20: import java.util.concurrent.atomic.AtomicInteger;
> unused
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java:
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java@25
PS1, Line 25: public class CatalogObjectVersionManager {
> What's the purpose of this class? Do changes need to be atomic with respect
Added class comment. Changes are atomic with respect to the global version but this class is not responsible for enforcing that. All the calls to the methods of this class originate from the synchronized ImpaladCatalog::updateCatalog() call that is also responsible for updating the global version.
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@129
PS1, Line 129: private final ReentrantReadWriteLock catalogLock_ = new ReentrantReadWriteLock(true);
> How do you feel about renaming this to versionLock_ to make it clear that t
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@159
PS1, Line 159: // Version of the last sent catalog update. The version of an update is the
> Maybe I'm being dense, but I don't understand what this means. Can you clar
Rephrased it. Let me know if it's clear now.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@170
PS1, Line 170: private int numCatalogUpdatesToGc_ = CATALOG_UPDATE_LOG_GC_FREQUENCY;
> What's the unit?
This is quite tricky. Let's talk offline.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@180
PS1, Line 180: private static class CatalogUpdateLogEntry {
> I think we need a better name since 'update' is not very telling as to what
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@209
PS1, Line 209: if (!(other instanceof CatalogUpdateLogEntry)) return false;
> if (this.getClass() != other.getClass()) return false;
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@222
PS1, Line 222: // To prevent the log from growing indefinitely, the oldest entries
> It's not obvious to me why a time-based eviction is correct. Don't we requi
There is only one case where this can cause problems. In particular, if a thread performing an operation with SYNC_DDL (e.g. add table) hangs after the change has been applied to the catalog but before the SYNC_DDL version is checked. If the hang lasts for more than CATALOG_UPDATE_LOG_FREQUENCY * catalog_update_frequency (sec), then the PublishedObjectLogEntry for that operation will be gc-ed before it is checked, causing the thread to hang. As a failsafe mechanism, we need to have a timeout while waiting for the SYNC_DDL version which will handle this and any other weird case like this.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@228
PS1, Line 228: private final ConcurrentHashMap<String, CatalogUpdateLogEntry> catalogUpdateLog_ =
> Do we have a user-facing command (invalidate metadata?) that clears this lo
Currently, we don't have anything like that. The only operation that purges the log is the gc.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@231
PS1, Line 231: private final Object catalogUpdateEventNotifier_ = new Object();
> Brief comment describing who waits and who notifies.
Added a comment. I think I am going to leave it here for now. It may be better to simply describe the semantics/behavior of this thing in each class. No strong opinion though.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@316
PS1, Line 316: private final boolean resetVersions_;
> incrementVersions_ and need a comment
Renamed and added a comment. We need this during INVALIDATE METADATA because we need to make sure that every catalog object gets a new version. We don't need this to be true in the normal case (periodic check and update of catalog) as it will result in unnecessary updates in a catalog delta.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@375
PS1, Line 375: cachePool.setCatalogVersion(incrementAndGetCatalogVersion());
> Comment on the purpose of this. Is it to handle cache pools that were delet
See comment above.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@406
PS1, Line 406: * Identifies and returns the catalog objects that were added/modified/deleted in the
> For our own sanity we need to document the versioning scheme and the variou
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@456
PS1, Line 456: lastSentCatalogUpdate_.set(toVersion);
> Is the placement of this statement within this function important? I assume
Yes, it needs to be before L470. Moved it closer to that.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@470
PS1, Line 470: synchronized (catalogUpdateEventNotifier_) {
> catalogPublicationNotifier_?
Renamed to topicUpdateEventNotifier_.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@606
PS1, Line 606: new CatalogUpdateLogEntry(updateEntry.getNumSkippedCatalogUpdates() + 1,
> Why not increment the skipped counter in place? I know the answer has to do
Actually, the reason I am doing it like that is to avoid the need for synchronization. The topicUpdateLogEntries are immutable.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1083
PS1, Line 1083: public void reset(TCatalogUpdateResult res) throws CatalogException {
> Instead of passing the 'res' could we return the version? The caller could
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1084
PS1, Line 1084: LOG.info("Invalidating all metadata.");
> Move this below the version grab to also log the version
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1799
PS1, Line 1799: * for in order to ensure that its result set ('result') has been broadcasted to all the
> broadcast
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1806
PS1, Line 1806: // TODO: Should we set a threshold to eventually break out of this loop and throw an
> Shouldn't it be possible to bound the number of times we need to loop? Ever
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1808
PS1, Line 1808: while (true) {
> while (versionToWaitFor == -1)?
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1811
PS1, Line 1811: catalogUpdateEventNotifier_.wait(CATALOG_UPDATE_WAIT_TIMEOUT_MS);
> * Why wake up based on a timer?
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1819
PS1, Line 1819: if (!result.isSetUpdated_catalog_objects() &&
> simpler to check this outside of the loop
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1830
PS1, Line 1830: updateVersion = getCoveringCatalogUpdate(result.getRemoved_catalog_objects());
> please use new var
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1843
PS1, Line 1843: * Returns the version of the catalog update that covers a set of TCatalogObjects.
> We should motivate this solution by explaining the SYNC_DDL problem with ou
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1852
PS1, Line 1852: private long getCoveringCatalogUpdate(List<TCatalogObject> tCatalogObjects) {
> getCoveringCatalogUpdateVersion
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1864
PS1, Line 1864: // CATALOG_UPDATE_LOG_GC_FREQUENCY updates and hence its entry was garbage
> It feels like this might lead to an infinite wait in getCatalogVersionToWai
We added the logic in waitForSyncDdlVersion to eventually exit from these kind of situations.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@331
PS1, Line 331: // impalad which will wait until this catalog update has been broadcasted to all the
> broadcast
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@335
PS1, Line 335: catalog_.getCatalogVersionToWaitFor(response.getResult()));
> waitForSyncDdlVersion() instead of getCatalogVersionToWaitFor()?
Done
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3102
PS1, Line 3102: resp.getResult().setVersion(catalog_.getCatalogVersionToWaitFor(resp.getResult()));
> Would be good to have a timer in getCatalogVersionToWaitFor() to log how lo
Done
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Nov 2017 18:26:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 1:
(39 comments)
I think this is going in the right direction. I was not able to spot any major flaws/bugs. There are details to be ironed out but the approach looks sound.
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.h@336
PS1, Line 336: /// Wait until the last applied catalog update has been broadcasted to the
broadcast
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.h@337
PS1, Line 337: /// entire cluster or until the catalog service id has changed.
entire cluster -> all coordinators
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc@1462
PS1, Line 1462: // TODO: What about query cancellation?
Unfortunately, most DDL operations are not cancellable due to IMPALA-2568 so I don't think we can reasonably address this.
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc@1503
PS1, Line 1503: const TUniqueId& catalog_service_id = catalog_update_result.catalog_service_id;
Document the waiting process for SYNC_DDL dance in the function comment of ProcessCatalogUpdateResult() in .h
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc@1507
PS1, Line 1507: // 'catalog_update_result' to determined when the effects of this operation
to determine
http://gerrit.cloudera.org:8080/#/c/8545/1/be/src/service/impala-server.cc@1512
PS1, Line 1512: WaitForCatalogUpdate(catalog_update_result.version, catalog_service_id);
What operations go down this path?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@89
PS1, Line 89: CatalogObjectVersionManager.INSTANCE.removeCatalogObjectVersion(
What's this new code for?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@20
PS1, Line 20: import java.util.concurrent.atomic.AtomicInteger;
unused
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java:
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java@25
PS1, Line 25: public class CatalogObjectVersionManager {
What's the purpose of this class? Do changes need to be atomic with respect to the global version?
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@129
PS1, Line 129: private final ReentrantReadWriteLock catalogLock_ = new ReentrantReadWriteLock(true);
How do you feel about renaming this to versionLock_ to make it clear that the lock is used to ensure that mutations to internal structures are atomic with respect to their corresponding version increase.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@159
PS1, Line 159: // Version of the last sent catalog update. The version of an update is the
Maybe I'm being dense, but I don't understand what this means. Can you clarify/rephrase?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@170
PS1, Line 170: private int numCatalogUpdatesToGc_ = CATALOG_UPDATE_LOG_GC_FREQUENCY;
What's the unit?
I feel like we should be able to make this a function of MAX_NUM_SKIPPED_CATALOG_UPDATES.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@180
PS1, Line 180: private static class CatalogUpdateLogEntry {
I think we need a better name since 'update' is not very telling as to what was updated. How about something along the lines of PublishedObjectLogEntry - that makes it clear the log contains entries published to the statestore via getCatalogDelta().
Ideally we would consistently stick to one term to describe the process of publishing updates to the statestore, and I'm not sure "catalog update" is crisp enough. What do you think?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@209
PS1, Line 209: if (!(other instanceof CatalogUpdateLogEntry)) return false;
if (this.getClass() != other.getClass()) return false;
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@222
PS1, Line 222: // To prevent the log from growing indefinitely, the oldest entries
It's not obvious to me why a time-based eviction is correct. Don't we require these log entries for certain operations with sync_ddl?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@228
PS1, Line 228: private final ConcurrentHashMap<String, CatalogUpdateLogEntry> catalogUpdateLog_ =
Do we have a user-facing command (invalidate metadata?) that clears this log in case we get into a weird state?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@231
PS1, Line 231: private final Object catalogUpdateEventNotifier_ = new Object();
Brief comment describing who waits and who notifies.
We could put this into Catalog (ImpalaCatalog has this thing too), but the different meanings might be confusing. Something to consider.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@316
PS1, Line 316: private final boolean resetVersions_;
incrementVersions_ and need a comment
do we really need this flag?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@375
PS1, Line 375: cachePool.setCatalogVersion(incrementAndGetCatalogVersion());
Comment on the purpose of this. Is it to handle cache pools that were deleted and recreated?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@406
PS1, Line 406: * Identifies and returns the catalog objects that were added/modified/deleted in the
For our own sanity we need to document the versioning scheme and the various logs in the class comment. What do the logs contain, how are they updated, what is their purpose, etc.
In particular, let's explicitly document the atomicity assumptions/requirements with respect to modifying the cache and various auxiliary structures like logs and the version counter.
We should also be clear that getCatalogDelta() collects a subset of the objects with version > 'fromVersion' and <= 'toVersion' and discuss when we skip objects and how we deal with them.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@456
PS1, Line 456: lastSentCatalogUpdate_.set(toVersion);
Is the placement of this statement within this function important? I assume we need to do it before notifyAll() below, anything else?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@470
PS1, Line 470: synchronized (catalogUpdateEventNotifier_) {
catalogPublicationNotifier_?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@598
PS1, Line 598: CatalogUpdateLogEntry updateEntry = getCatalogUpdateLogEntry(tbl.getUniqueName());
The name updateEntry is difficult to understand, how about lastPub or similar
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@606
PS1, Line 606: new CatalogUpdateLogEntry(updateEntry.getNumSkippedCatalogUpdates() + 1,
Why not increment the skipped counter in place? I know the answer has to do with getCatalogUpdateLogEntry(). I'm thinking that getCatalogUpdateLogEntry() makes the code more difficult to understand and does not buy much code simplification (it avoids a null check in 2 places)
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1083
PS1, Line 1083: public void reset(TCatalogUpdateResult res) throws CatalogException {
Instead of passing the 'res' could we return the version? The caller could update its 'res' if needed.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1084
PS1, Line 1084: LOG.info("Invalidating all metadata.");
Move this below the version grab to also log the version
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1799
PS1, Line 1799: * for in order to ensure that its result set ('result') has been broadcasted to all the
broadcast
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1806
PS1, Line 1806: // TODO: Should we set a threshold to eventually break out of this loop and throw an
Shouldn't it be possible to bound the number of times we need to loop? Every object can be skipped at most a fixed number of times.
Looping forever makes me nervous, we definitely need a way to get out of here in case of bugs.
For non-invalidate operations I think the max number of loops should be:
updatedObjects.size() * (MAX_NUM_SKIPPED_CATALOG_UPDATES + 1)
We don't skip deletions right?
For invalidate operations we may have to think of something else.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1808
PS1, Line 1808: while (true) {
while (versionToWaitFor == -1)?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1811
PS1, Line 1811: catalogUpdateEventNotifier_.wait(CATALOG_UPDATE_WAIT_TIMEOUT_MS);
* Why wake up based on a timer?
* Should this check ideally not come at the end of the loop to avoid waiting unnecessarily in case the objects have already been published by the time this function is called?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1819
PS1, Line 1819: if (!result.isSetUpdated_catalog_objects() &&
simpler to check this outside of the loop
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1830
PS1, Line 1830: updateVersion = getCoveringCatalogUpdate(result.getRemoved_catalog_objects());
please use new var
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1843
PS1, Line 1843: * Returns the version of the catalog update that covers a set of TCatalogObjects.
We should motivate this solution by explaining the SYNC_DDL problem with our new fuzzy checkpointing scheme in the class comment.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1852
PS1, Line 1852: private long getCoveringCatalogUpdate(List<TCatalogObject> tCatalogObjects) {
getCoveringCatalogUpdateVersion
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1864
PS1, Line 1864: // CATALOG_UPDATE_LOG_GC_FREQUENCY updates and hence its entry was garbage
It feels like this might lead to an infinite wait in getCatalogVersionToWaitFor(). I bet that doesn't happen in practice because CATALOG_UPDATE_LOG_GC_FREQUENCY has such a big value, but it feels like we could do something more bounded/principled here to clarify the behavior.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@a1553
PS1, Line 1553:
Lol, what is this?! Good riddance.
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@331
PS1, Line 331: // impalad which will wait until this catalog update has been broadcasted to all the
broadcast
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@335
PS1, Line 335: catalog_.getCatalogVersionToWaitFor(response.getResult()));
waitForSyncDdlVersion() instead of getCatalogVersionToWaitFor()?
http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3102
PS1, Line 3102: resp.getResult().setVersion(catalog_.getCatalogVersionToWaitFor(resp.getResult()));
Would be good to have a timer in getCatalogVersionToWaitFor() to log how long we waited.
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 00:49:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@596
PS1, Line 596: addTableToCatalogDeltaHelper(tbl, fromVersion, toVersion, resp);
> Maybe I'm misunderstanding something but I still see the lock contention on
Just to be clear, when I said it is stuck in tryLockTable(), I meant the following,
if (tryLockTable(foo)) {
try {
newCatalogVersion = catalog_.incrementAndGetCatalogVersion();
catalog_.getLock().writeLock().unlock();
// do the actual DDL op
// example alterTable(foo) <---- stuck at this point
} finally {
tbl.unlock()
}
}
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 06:18:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8545/1/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/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@596
PS1, Line 596: addTableToCatalogDeltaHelper(tbl, fromVersion, toVersion, resp);
> I think your assessment is correct.
I see, thanks for the explanation. I thought we are trying to make getCatalogDelta() non-conflicting too, since this contention can delay other Catalog updates like table loads etc. I understand the scope of this change better now, I have other comments that I'll flush out separately.
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 07:00:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Patch Set 2:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/8545/2/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/8545/2/be/src/service/client-request-state.cc@187
PS2, Line 187: reset_req.reset_metadata_params.__set_sync_ddl(
> Why do we need to set sync_ddl twice?
It's because we're not always consistent in how we process different requests. TCatalogOpRequest::sync_ddl is now a required field. At the same time some operations process the *params field and don't have access to the parent TCatalogOpRequest, so the value of the sync_ddl flag is lost.
http://gerrit.cloudera.org:8080/#/c/8545/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8545/2/be/src/service/impala-server.cc@1430
PS2, Line 1430: LOG(INFO) << "Update applied with version: " << new_catalog_version
> Catalog update applied...
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java:
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java@29
PS2, Line 29: public class CatalogObjectVersionManager {
> Name is not very telling. Some alternatives:
Went with option #1.
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionManager.java@37
PS2, Line 37: public void updateCatalogObjectVersions(long oldVersion, long newVersion) {
> Do you think it's worth checking the return codes of the calls to objectVer
Yes, added check on the remove path. The version should correspond to an existing object that is about to be removed, so the version should exist in the queue.
http://gerrit.cloudera.org:8080/#/c/8545/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/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@86
PS2, Line 86: * and processing of DDL requests. For each DDL request, the CatalogServiceCatalog
> This description of the versioning scheme is not accurate anymore.
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@96
PS2, Line 96: * - Delete log. The delete log records catalog objects that have been removed from the
> Let's give an intuition for the purpose of this log, e.g.:
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@97
PS2, Line 97: * catalog. An entry is added to this log every time an object is removed (e.g.
> An entry with a new version is added ...
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@99
PS2, Line 99: * be perfomed atomically. An entry is removed from this log from the topic update
> removed from this log by the topic update thread
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@102
PS2, Line 102: * been processed by the topic update thread. The topic update thread is the only one
> that have been included in a topic update.
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@105
PS2, Line 105: * Information per catalog object includes the number of times a catalog object has
> Each entry includes the number of times a catalog object has been omitted i
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@111
PS2, Line 111: * version that the issueing impalad must wait for in order to ensure that the effects
> typo: issuing
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@112
PS2, Line 112: * of this operation have been broadcast to all the coordinators. The time-based cleanup
> Why did we chose a time-based cleanup then? I think the time-based cleanup
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@129
PS2, Line 129: * that should be employed if both the catalog lock and table locks need to be held at
> catalog lock -> version lock?
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@183
PS2, Line 183: // Version of the last topic update sent to the statestore.
> sent -> returned (to make the control flow clearer)
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@192
PS2, Line 192: private final static int TOPIC_UPDATE_LOG_GC_FREQUENCY = 50;
> Mention how this affects the lifetime of an individual entry. My understand
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@195
PS2, Line 195: p
> Let's create a TopicUpdateLog class that encapsulates these counters and th
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@258
PS2, Line 258: private final Object topicUpdateEventNotifier_ = new Object();
> Maybe we should use the topicUpdateLog_ for this purpose once we have a sep
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1228
PS2, Line 1228: private void removeDbHelper(Db db) {
> updateDeleteLog(Db db)?
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1938
PS2, Line 1938: topicUpdateLog_.get(CatalogDeltaLog.toCatalogObjectKey(tCatalogObject));
> Move toCatalogObjectKey() to Catalog, referring to the delta log here is a
Done
http://gerrit.cloudera.org:8080/#/c/8545/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1976
PS2, Line 1976: if (topicUpdateLog_.remove(entry.getKey(), entry.getValue())) {
> Are uou sure it's ok to remove() while iterating over the map? Might be cle
It's a ConcurrentHashMap, so it doesn't care for concurrent modifications. Besides, I don't want to just use remove on an iterator because I need to check if the value of the to-be-removed object has changed and abort if that is the case. The last two operations need to be performed atomically and iterator.remove does not provide that functionality.
http://gerrit.cloudera.org:8080/#/c/8545/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/8545/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@179
PS2, Line 179: // functions and priviles) and then the top-level objects (databases and roles).
> typo: privlieges
Done
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 2
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Dec 2017 23:49:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has abandoned this change. ( http://gerrit.cloudera.org:8080/8545 )
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
Abandoned
Resubmitted merged with the changes for impala-5538.
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 3
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8545
to look at the new patch set (#3).
Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
......................................................................
[PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
Problem: A long running table metadata operation (e.g. refresh) could
prevent any other metadata operation from making progress if it
coincided with the gather catalog updates operation. The problem was due
to the conservative locking scheme used when catalog updates were
collected. In particular, in order to collect a consistent snapshot of
metadata changes, the global catalog lock was held for the entire
duration of that operation.
Solution: To improve the concurrency of catalog operations the following
changes are performed:
* A range of catalog versions determines the catalog changes to be
included in a catalog update. Any catalog changes that do not fall in
the specified range are ignored (to be processed in subsequent catalog
updates).
* The catalog allows metadata operations to make progress while collecting
catalog updates.
* To prevent starvation of catalog updates (i.e. frequently updated
catalog objects skipping catalog updates indefinitely), we keep track
of the number of times a catalog object has skipped an update and if
that number exceeds a threshold it is included in the next catalog
update even if its version is not in the specified catalog update
version range. Hence, the same catalog object may be sent in two
consecutive catalog updates.
Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
---
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/CatalogService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java
A fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
A fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionQueue.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/DataSource.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Role.java
M fe/src/main/java/org/apache/impala/catalog/RolePrivilege.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
29 files changed, 1,417 insertions(+), 587 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/8545/3
--
To view, visit http://gerrit.cloudera.org:8080/8545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 3
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>