You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/06/06 22:28:40 UTC
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Hello Vuk Ercegovac,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10627
to review the following change.
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
IMPALA-7135. Skeleton implementation of LocalCatalog
This adds some of the high level classes for implementing the local
catalog:
- LocalCatalog is the top level implementation. The plan is to
instantiate this once per query, so that no thread safety is required.
- It loads metadata from a MetaProvider interface. The current
implementation fetches directly from HMS and provides no caching. A
future subtask will add a CachingMetaProvider implementation.
Separating out caching will make it easier to experiment with
different policies or storage mechanisms.
- It instantiates LocalDb and LocalTable objects to implement FeDb and
FeTable. These are mostly stubbed out except for the most basic
functionality. Functionality will be filled in incrementally in
further patches.
Since it's not yet possible to hook this up to most of the existing
tests, a very simple new unit test is included to cover the bits of
functionality that are not stubbed out. I didn't concentrate on too much
test coverage here, since once we've implemented more functionality we
can switch over all of the existing tests to get coverage of the new
implementation.
Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
A fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalogException.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
A fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
A fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
8 files changed, 765 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10627/1
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Jun 2018 20:08:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2682/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Jun 2018 00:50:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Jun 2018 00:50:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10627
to look at the new patch set (#3).
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
IMPALA-7135. Skeleton implementation of LocalCatalog
This adds some of the high level classes for implementing the local
catalog:
- LocalCatalog is the top level implementation. The plan is to
instantiate this once per query, so that no thread safety is required.
- It loads metadata from a MetaProvider interface. The current
implementation fetches directly from HMS and provides no caching. A
future subtask will add a CachingMetaProvider implementation.
Separating out caching will make it easier to experiment with
different policies or storage mechanisms.
- It instantiates LocalDb and LocalTable objects to implement FeDb and
FeTable. These are mostly stubbed out except for the most basic
functionality. Functionality will be filled in incrementally in
further patches.
Since it's not yet possible to hook this up to most of the existing
tests, a very simple new unit test is included to cover the bits of
functionality that are not stubbed out. I didn't concentrate on too much
test coverage here, since once we've implemented more functionality we
can switch over all of the existing tests to get coverage of the new
implementation.
Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java
M fe/src/main/java/org/apache/impala/catalog/FeDb.java
A fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalogException.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
A fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
A fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
10 files changed, 794 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10627/3
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10627/3/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/10627/3/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@35
PS3, Line 35: * (filesystem, HMS, etc with no caching.
> nit: ")"
Done
http://gerrit.cloudera.org:8080/#/c/10627/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/10627/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@30
PS3, Line 30: into LocalCatalog
> Is it required/intended to be so specific? Nothing here restricts which pro
Done
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Jun 2018 00:11:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Mostafa Mokhtar, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10627
to look at the new patch set (#4).
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
IMPALA-7135. Skeleton implementation of LocalCatalog
This adds some of the high level classes for implementing the local
catalog:
- LocalCatalog is the top level implementation. The plan is to
instantiate this once per query, so that no thread safety is required.
- It loads metadata from a MetaProvider interface. The current
implementation fetches directly from HMS and provides no caching. A
future subtask will add a CachingMetaProvider implementation.
Separating out caching will make it easier to experiment with
different policies or storage mechanisms.
- It instantiates LocalDb and LocalTable objects to implement FeDb and
FeTable. These are mostly stubbed out except for the most basic
functionality. Functionality will be filled in incrementally in
further patches.
Since it's not yet possible to hook this up to most of the existing
tests, a very simple new unit test is included to cover the bits of
functionality that are not stubbed out. I didn't concentrate on too much
test coverage here, since once we've implemented more functionality we
can switch over all of the existing tests to get coverage of the new
implementation.
Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java
M fe/src/main/java/org/apache/impala/catalog/FeDb.java
A fe/src/main/java/org/apache/impala/catalog/HasName.java
A fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalogException.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
A fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
A fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
11 files changed, 823 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10627/4
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2660/
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Jun 2018 21:59:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Jun 2018 04:10:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Mostafa Mokhtar, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10627
to look at the new patch set (#5).
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
IMPALA-7135. Skeleton implementation of LocalCatalog
This adds some of the high level classes for implementing the local
catalog:
- LocalCatalog is the top level implementation. The plan is to
instantiate this once per query, so that no thread safety is required.
- It loads metadata from a MetaProvider interface. The current
implementation fetches directly from HMS and provides no caching. A
future subtask will add a CachingMetaProvider implementation.
Separating out caching will make it easier to experiment with
different policies or storage mechanisms.
- It instantiates LocalDb and LocalTable objects to implement FeDb and
FeTable. These are mostly stubbed out except for the most basic
functionality. Functionality will be filled in incrementally in
further patches.
Since it's not yet possible to hook this up to most of the existing
tests, a very simple new unit test is included to cover the bits of
functionality that are not stubbed out. I didn't concentrate on too much
test coverage here, since once we've implemented more functionality we
can switch over all of the existing tests to get coverage of the new
implementation.
Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java
M fe/src/main/java/org/apache/impala/catalog/FeDb.java
A fe/src/main/java/org/apache/impala/catalog/HasName.java
A fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalogException.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
A fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
A fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
11 files changed, 823 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10627/5
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Jun 2018 01:19:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10627/3/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/10627/3/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@35
PS3, Line 35: * (filesystem, HMS, etc with no caching.
nit: ")"
http://gerrit.cloudera.org:8080/#/c/10627/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/10627/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@30
PS3, Line 30: into LocalCatalog
Is it required/intended to be so specific? Nothing here restricts which process reads metadata or what it does with it. This looks more like a federation api. I'd just replace the last two words with a "see LocalCatalog for an example".
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 20:43:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 2:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@49
PS2, Line 49:
: new MetaStor
nit: single line
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@53
PS2, Line 53: which does not interact with catalogd
Although it could, if a hypothetical CatalogdMetaProvider is implemented (along with needed catalogd changes). The design does not preclude it, afaict? Lets drop this last part, since in time, it will become stale and/or the context may be lost.
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@72
PS2, Line 72: metaProvider
precondition to assert non-null.
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@76
PS2, Line 76: matcher
where's this used?
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@88
PS2, Line 88: list
nit: names (seems more intuitive)
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@44
PS2, Line 44: name_
lowercase?
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@53
PS2, Line 53: catalog, String dbName
precondition for non-null
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@70
PS2, Line 70: c
nit: C
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@71
PS2, Line 71: e);
nit: pull up to prev. line
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@85
PS2, Line 85: assert tb
use precondition, and check non-null.
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@122
PS2, Line 122: c
nit: C
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@42
PS2, Line 42: name_
lowercase?
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@33
PS2, Line 33: * or may include caching, etc.
so far, this covers a portion of hms. pls add a todo for covering other hms apis, fs, and sentry.
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Jun 2018 05:33:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10627
to look at the new patch set (#2).
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
IMPALA-7135. Skeleton implementation of LocalCatalog
This adds some of the high level classes for implementing the local
catalog:
- LocalCatalog is the top level implementation. The plan is to
instantiate this once per query, so that no thread safety is required.
- It loads metadata from a MetaProvider interface. The current
implementation fetches directly from HMS and provides no caching. A
future subtask will add a CachingMetaProvider implementation.
Separating out caching will make it easier to experiment with
different policies or storage mechanisms.
- It instantiates LocalDb and LocalTable objects to implement FeDb and
FeTable. These are mostly stubbed out except for the most basic
functionality. Functionality will be filled in incrementally in
further patches.
Since it's not yet possible to hook this up to most of the existing
tests, a very simple new unit test is included to cover the bits of
functionality that are not stubbed out. I didn't concentrate on too much
test coverage here, since once we've implemented more functionality we
can switch over all of the existing tests to get coverage of the new
implementation.
Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
A fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalogException.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
A fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
A fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
8 files changed, 766 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10627/2
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 2:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@49
PS2, Line 49:
: new MetaStor
> nit: single line
Done
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@53
PS2, Line 53: which does not interact with catalogd
> Although it could, if a hypothetical CatalogdMetaProvider is implemented (a
good point. I'll rephrase this to say that this implements a "pull-based" catalog design -- the source might be the direct metadata sources, or an intermediate node like catalogd.
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@72
PS2, Line 72: metaProvider
> precondition to assert non-null.
Done
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@76
PS2, Line 76: matcher
> where's this used?
woops, good catch! copy paste fail. Fixed and added tests.
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@88
PS2, Line 88: list
> nit: names (seems more intuitive)
Done
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@44
PS2, Line 44: name_
> lowercase?
added docs
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@53
PS2, Line 53: catalog, String dbName
> precondition for non-null
Done
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@71
PS2, Line 71: e);
> nit: pull up to prev. line
Done
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@85
PS2, Line 85: assert tb
> use precondition, and check non-null.
Done
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@122
PS2, Line 122: c
> nit: C
Done
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@42
PS2, Line 42: name_
> lowercase?
Done
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@33
PS2, Line 33: * or may include caching, etc.
> so far, this covers a portion of hms. pls add a todo for covering other hms
Done
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 00:14:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
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/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
IMPALA-7135. Skeleton implementation of LocalCatalog
This adds some of the high level classes for implementing the local
catalog:
- LocalCatalog is the top level implementation. The plan is to
instantiate this once per query, so that no thread safety is required.
- It loads metadata from a MetaProvider interface. The current
implementation fetches directly from HMS and provides no caching. A
future subtask will add a CachingMetaProvider implementation.
Separating out caching will make it easier to experiment with
different policies or storage mechanisms.
- It instantiates LocalDb and LocalTable objects to implement FeDb and
FeTable. These are mostly stubbed out except for the most basic
functionality. Functionality will be filled in incrementally in
further patches.
Since it's not yet possible to hook this up to most of the existing
tests, a very simple new unit test is included to cover the bits of
functionality that are not stubbed out. I didn't concentrate on too much
test coverage here, since once we've implemented more functionality we
can switch over all of the existing tests to get coverage of the new
implementation.
Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Reviewed-on: http://gerrit.cloudera.org:8080/10627
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java
M fe/src/main/java/org/apache/impala/catalog/FeDb.java
A fe/src/main/java/org/apache/impala/catalog/HasName.java
A fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalCatalogException.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
A fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
A fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
A fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
11 files changed, 823 insertions(+), 6 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 8
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7135. Skeleton implementation of LocalCatalog
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee
Gerrit-Change-Number: 10627
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Jun 2018 16:31:30 +0000
Gerrit-HasComments: No