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-25935: Cleanup IMetaStoreClient#getPartitionsByNames APIs #3072

Merged
merged 7 commits into from Mar 10, 2022
Merged

HIVE-25935: Cleanup IMetaStoreClient#getPartitionsByNames APIs #3072

merged 7 commits into from Mar 10, 2022

Conversation

pvary
Copy link
Contributor

@pvary pvary commented Mar 2, 2022

What changes were proposed in this pull request?

Cleans up deprecated methods

Why are the changes needed?

We do not want to release deprecated methods

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit tests will show errors, if any

@pvary pvary requested a review from zabetak March 3, 2022 08:08
dengzhhu653
dengzhhu653 reviewed Mar 5, 2022
View reviewed changes
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
validWriteIdList = vWriteIdList != null ? vWriteIdList.toString() : null;
tableId = tbl.getTTable().getId();
}
String validWriteIdList = null;
Copy link
Member

Choose a reason for hiding this comment

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

nit: I wonder if we can use the following:

 GetPartitionsByNamesRequest req = MetaStoreUtils.convertToGetPartitionsByNamesRequest(tbl.getDbName(), tbl.getTableName(),
            partNames.subList(nBatches*batchSize, nParts), getColStats, Constants.HIVE_ENGINE, null,
            null);
 List<org.apache.hadoop.hive.metastore.api.Partition> tParts = getPartitionsByNames(req, tbl);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to push down the vaildWriteIdList and the tableId, so if there is caching in the catalog side then we the cache could be refreshed when needed

@dengzhhu653
Copy link
Member

Overall looks good to me, +1

Disable again TestStats.partitionedTableDeprecatedCalls. This was dis…
99553cc 
…abled before the changes, so I think this should be ok
dengzhhu653
dengzhhu653 reviewed Mar 9, 2022
View reviewed changes
...one-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestStats.java Outdated
@@ -291,7 +285,6 @@ public void tableInHiveCatalog() throws TException {
dropStats(DEFAULT_CATALOG_NAME, dbName, tableName, null, colMap.keySet());
}

@Ignore("HIVE-19509: Disable tests that are failing continuously")
@Test
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should also disable this method...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe all 3. Will do it when I have some time

@pvary
Copy link
Contributor Author

pvary commented Mar 9, 2022

@dengzhhu653, @zabetak: Could you please review?
Thanks,
Peter

@dengzhhu653
Copy link
Member

@dengzhhu653, @zabetak: Could you please review? Thanks, Peter

Look good to me, +1

@dengzhhu653
Copy link
Member

@dengzhhu653, @zabetak: Could you please review? Thanks, Peter

Look good to me, +1

IMetaStoreClient listPartitionNames at line 1309 can be cleaned, it would be great if you can do me a favor for that.
Thanks,
Zhihua Deng

@pvary
Copy link
Contributor Author

pvary commented Mar 10, 2022 •
edited by dengzhhu653

IMetaStoreClient listPartitionNames at line 1309 can be cleaned, it would be great if you can do me a favor for that.
Thanks,
Zhihua Deng

Could you please ellaborate?

@pvary pvary merged commit e87967a into apache:master Mar 10, 2022
@dengzhhu653
Copy link
Member

The List<String> listPartitionNames(String catName, String dbName, String tblName, String defaultPartName, byte[] exprBytes, String order, short maxParts) method was added by me as part of HIVE-22458.
Currently in master we use List<String> listPartitionNames(PartitionsByExprRequest request) to replace others with the same method name, so I think the method can also be cleaned...

Sorry about that...

@pvary
Copy link
Contributor Author

pvary commented Mar 10, 2022

The List<String> listPartitionNames(String catName, String dbName, String tblName, String defaultPartName, byte[] exprBytes, String order, short maxParts) method was added by me as part of HIVE-22458. Currently in master we use List<String> listPartitionNames(PartitionsByExprRequest request) to replace others with the same method name, so I think the method can also be cleaned...

Created https://issues.apache.org/jira/browse/HIVE-26025

DongWei-4 pushed a commit to DongWei-4/hive that referenced this pull request Oct 28, 2022
@DongWei-4
HIVE-25935: Cleanup IMetaStoreClient#getPartitionsByNames APIs (Peter…
8197ab9 
… Vary reviewed by Zhihua Deng) (apache#3072)
dengzhhu653 pushed a commit to dengzhhu653/hive that referenced this pull request Dec 15, 2022
@dengzhhu653
HIVE-25935: Cleanup IMetaStoreClient#getPartitionsByNames APIs (Peter…
c2b01fc 
… Vary reviewed by Zhihua Deng) (apache#3072)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dengzhhu653 dengzhhu653 dengzhhu653 approved these changes

@zabetak zabetak Awaiting requested review from zabetak

Assignees
No one assigned
Labels
tests passed
Projects
None yet
Milestone
No milestone
3 participants
@pvary @dengzhhu653 @kgyrtkirk