Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Hive: Introduce HiveMetastoreExtension for Hive tests #9282

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Hive: Introduce HiveMetastoreExtension for Hive tests #9282

merged 3 commits into from
Dec 12, 2023

Conversation

nk1506
Copy link
Contributor

@nk1506 nk1506 commented Dec 12, 2023 •
edited
Loading

CC: @nastra

@github-actions github-actions bot added the hive label Dec 12, 2023
@nk1506 nk1506 mentioned this pull request Dec 12, 2023
Merged
nastra
nastra requested changes Dec 12, 2023
View reviewed changes
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableBaseTest.java Outdated Show resolved Hide resolved
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableBaseTest.java Outdated Show resolved Hide resolved
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommitLocks.java Outdated Show resolved Hide resolved
hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java Show resolved Hide resolved
hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java Outdated Show resolved Hide resolved
hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java Outdated Show resolved Hide resolved
hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java Outdated Show resolved Hide resolved
@nastra nastra changed the title Hive: Use jUnit5 based HiveMetastoreExtension with Hive catalog tests Hive: Introduce HiveMetastoreExtension for Hive tests Dec 12, 2023
nastra
nastra approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @nk1506, this LGTM

@ajantha-bhat
Copy link
Member

Let me take a look

@ajantha-bhat ajantha-bhat self-requested a review December 12, 2023 10:30
ajantha-bhat
ajantha-bhat reviewed Dec 12, 2023
View reviewed changes
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java Outdated Show resolved Hide resolved
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java
@BeforeEach
public void createTableLocation() throws IOException {
catalog =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should initialize the catalog only once. Not for each tests as nothing changes between tests related to catalog config.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can we move this to HiveMetastoreExtension and get the catalog from there? It can help in duplicating this code in each test class which uses this extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As consistency with other catalog tests which initialise before each test and drop the catalog after every test. For Hive also the idea is same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can we move this to HiveMetastoreExtension and get the catalog from there? It can help in duplicating this code in each test class which uses this extension.

I don't think the catalog initialization should be moved into HiveMetastoreExtension. The sole purpose of HiveMetastoreExtension is to provide the metastore part

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nastra: Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've replied already above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wanted your thoughts on this comment
#9282 (comment)

ajantha-bhat
ajantha-bhat reviewed Dec 12, 2023
View reviewed changes
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java Outdated Show resolved Hide resolved
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java
@BeforeEach
public void createTableLocation() throws IOException {
catalog =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can we move this to HiveMetastoreExtension and get the catalog from there? It can help in duplicating this code in each test class which uses this extension.

hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java
TimeUnit.MILLISECONDS.sleep(EVICTION_INTERVAL - TimeUnit.SECONDS.toMillis(2));
HiveClientPool clientPool2 = clientPool.clientPool();
assertThat(clientPool2).isSameAs(clientPool1);
TimeUnit.MILLISECONDS.sleep(EVICTION_INTERVAL + TimeUnit.SECONDS.toMillis(5));
assertThat(
CachedClientPool.clientPoolCache()
.getIfPresent(CachedClientPool.extractKey(null, hiveConf)))
.getIfPresent(
CachedClientPool.extractKey(null, HIVE_METASTORE_EXTENSION.hiveConf())))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: some test we are extracting to a variable and in some test we are using directly. Let us have a unified style.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At other test hiveConf was being used at multiple places, that's the reason I created a local variable.

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommitLocks.java Outdated Show resolved Hide resolved
ajantha-bhat
ajantha-bhat approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@ajantha-bhat ajantha-bhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it is LGTM.

@nastra nastra merged commit 09b44bb into apache:main Dec 12, 2023
45 checks passed
lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024
@nk1506 @nastra @lisirrx
Hive: Introduce HiveMetastoreExtension for Hive tests (apache#9282)
7285847 
Co-authored-by: Eduard Tudenhoefner <[email protected]>
geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024
@nk1506 @nastra @geruh
Hive: Introduce HiveMetastoreExtension for Hive tests (apache#9282)
460a767 
Co-authored-by: Eduard Tudenhoefner <[email protected]>
@ajantha-bhat ajantha-bhat mentioned this pull request Mar 26, 2024
Merged
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
@nk1506 @nastra @devangjhabakh
Hive: Introduce HiveMetastoreExtension for Hive tests (apache#9282)
ebc286a 
Co-authored-by: Eduard Tudenhoefner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajantha-bhat ajantha-bhat ajantha-bhat approved these changes

@nastra nastra nastra approved these changes

Assignees
No one assigned
Labels
hive
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nk1506 @ajantha-bhat @nastra