impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Quanlong Huang (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-7506: support global INVALIDATE METADATA in local catalog mode
Date Sat, 19 Oct 2019 02:02:58 GMT
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14307
)

Change subject: IMPALA-7506: support global INVALIDATE METADATA in local catalog mode
......................................................................


Patch Set 6:

(4 comments)

Thanks Vihang's comments!

> I looks like you are sending all the objects in the catalogd when you detect the reset.
Is it possible to not send the objects and just invalidate the whole local catalog cache when
you detect that the last reset version is changed?

Reseting coordinator's local catalog is ok. But coordinator needs to know when the reset()
in catalogd finishs. For SYNC_DDL, it also needs to know the catalog topic version (not catalog
version, it's the version of the statestore topic) so it can wait for other coordinators to
be ready. I have an explanation with code links in "Why can’t we simply reset the cache
of local catalog mode Coordinator for global INVALIDATE METADATA?" in the doc: https://docs.google.com/document/d/1-AoigzsQPgSGosW4vtVP8E7TiwoJJfLkAq3Rd6KvKBg

http://gerrit.cloudera.org:8080/#/c/14307/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14307/5//COMMIT_MSG@41
PS5, Line 41:  1) No topic updates are sent from catalogd when the write lock of
            : versionLock is held in CatalogServiceCatalog.reset(). Note that the
            : update thread requires holding the read lock of versionLock.
            :  2) Authz changes before holding the write lock can only be sent in a
            : previous topic update or in the next topic update after reset().
            :  3) No catalog objects are skipped in the topic update right after
            : reset(). See changes in GetCatalogDeltaContext
> Did you consider the case when a reset is executed during execution of getC
Oh, it's possible when reset() runs again after reset(), so getCatalogDelta thread is running
with collectFullUpdates being set and will collect everything.

In normal cases if getCatalogDelta() and reset() run concurrently, it's ok since getCatalogDelta
runs with collectFullUpdates unset so will only collect updates in range of (fromVersion,
toVersion]. After reset() holding the write lock, the updates are with version larger than
toVersion.

I'll update the proof here. It's ok for a previous update to contain some results of the reset().
The most important update, CATALOG type TCatalog object, can only get the correct lastResetCatalogVersion_
after reset() finishes. Before that, it just gets an old value of lastResetCatalogVersion_.
(lastResetCatalogVersion_ is updated at the end of reset())


http://gerrit.cloudera.org:8080/#/c/14307/5/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/14307/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@721
PS5, Line 721:   public long getCatalogDelta(long nativeCatalogServerPtr, long fromVersion)
throws
             :       TException {
             :     long toVersion;
             :     boolean collectFullUpdates;
             :     versionLock_.readLock().lock();
             :     try {
             :       toVersion = catalogVersion_;
             :      
> what happens if the reset thread takes a write lock after this code block?
Then toVersion is small enough that won't cover updates after reset() acquiring the write
lock. However, if reset() runs again after reset(), hasResetCatalog is true so all updates
will be collected. As in the above comment, I think it's ok for a previous update to contain
some results of a running reset() as long as it don't propagate the lastResetCatalogVersion_.

BTW, with the changes in addTableToCatalogDeltaHelper(), in local catalog mode we no longer
need to acquire the table locks. So collecting all table updates won't be blocked by concurrent
DDLs.


http://gerrit.cloudera.org:8080/#/c/14307/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1104
PS5, Line 1104:    * which would also violate the semantics of SYNC_DDL.
> Does the collectFullUpdates flag come in play in v1 as well? If yes, that s
Yes, it should be used only in MINIMAL topic mode. I'll constrain this.


http://gerrit.cloudera.org:8080/#/c/14307/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1601
PS5, Line 1601: sing for id just befo
> It looks like the currentCatalogVersion may not be the right reset version 
Sorry, I should rename it to "lastResetCatalogVersion" or "lastResetBeginVersion" to avoid
confusion. It's the same version that we return to the coordinator, meaning that all catalog
objects with versions <= this will be invalidated. Coordinator first gets this version
in the RPC response, then when receiving this again in the CATALOG type TCatalog object update
(via statestore topic update), it knows that reset() has finished and all results are received.

Propagating (catalogVersion_-1) is not we want since it's the current version - 1 after reset().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61a7ab1ffa062620ffbc2dadc34bd7a8ca9e549
Gerrit-Change-Number: 14307
Gerrit-PatchSet: 6
Gerrit-Owner: Quanlong Huang <huangquanlong@gmail.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <huangquanlong@gmail.com>
Gerrit-Reviewer: Todd Lipcon <todd@apache.org>
Gerrit-Reviewer: Vihang Karajgaonkar <vihang@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Oct 2019 02:02:58 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message