> On Jan. 3, 2019, 6 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 4549-4555 (original), 4560-4569 (patched)
> > <https://reviews.apache.org/r/69642/diff/4/?file=2117200#file2117200line4560>
> >
> > Isn't simpler to have a constructor that accepts the (catName, dbName,
> > tblName, this) instead of using a Supplier? It will reduce code and make
> > the code more readable.
So I tried implementing with a constrcutor which accepts the catName, dbName,
tblName. That actually looks more difficult to understand.
The PreReadTableEvent would become the following
```java
public class PreReadTableEvent extends PreEventContext {
private final AtomicReference<Table> table;
private final String catName;
private final String dbName;
private final String tblName;
/**
* event with pre-fetched table object
* @param table the table object
* @param handler the HMS handler
*/
public PreReadTableEvent(Table table, IHMSHandler handler) {
super(PreEventType.READ_TABLE, handler);
this.table = new AtomicReference<>(table);
this.catName = null;
this.dbName = null;
this.tblName = null;
}
public PreReadTableEvent(String catName, String dbName, String tblName,
IHMSHandler handler) {
super(PreEventType.READ_TABLE, handler);
this.table = new AtomicReference<>(null);
this.catName = catName;
this.dbName = dbName;
this.tblName = tblName;
}
private Table getTableFromMs() {
try {
Table t = super.getHandler().getMS().getTable(catName, dbName, tblName);
if (t == null) {
throw new NoSuchObjectException(TableName.getQualified(catName, dbName,
tblName)
+ " table not found");
}
return t;
} catch (NoSuchObjectException | MetaException e) {
throw new RuntimeException(e);
}
}
/**
* @return the table
*/
public Table getTable() {
Table retVal = table.get();
if (retVal == null) {
retVal = getTableFromMs();
if (!table.compareAndSet(null, getTableFromMs())) {
return table.get();
}
}
return retVal;
}
}
```
In my opinion, this is harder to read and understand and prone to many errors.
We have multiple properties (catName, dbName and tblName) which can either be
null or not and depending on that the behavior is different. We also need to
have the slightly confusing implementation of getTable() to do the checks and
compareAndSet to ensure thread-safety. Suppliers.memoize(..) abstracts many of
this from us. I think my current approach is more readable and maintainable.
- Karthik
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69642/#review211643
-----------------------------------------------------------
On Jan. 3, 2019, 1:40 a.m., Karthik Manamcheri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69642/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2019, 1:40 a.m.)
>
>
> Review request for hive, Adam Holley, Na Li, Morio Ramdenbourg, Naveen
> Gangam, Peter Vary, Sergio Pena, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-20977: Lazy evaluate the table object in PreReadTableEvent to improve
> get_partition performance
>
>
> Diffs
> -----
>
>
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> a9398ae1e7
>
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/events/PreReadTableEvent.java
> beec72bc12
>
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/ThrowingSupplier.java
> PRE-CREATION
>
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
> 7429d18226
>
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
> fe64a91b56
>
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestGetPartitions.java
> 4d7f7c1220
>
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestListPartitions.java
> a338bd4032
>
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/events/TestPreReadTableEvent.java
> PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/69642/diff/4/
>
>
> Testing
> -------
>
> Unit tests.
> Manual performance test with Cloudera BDR to notice improved backup
> performance.
>
>
> Thanks,
>
> Karthik Manamcheri
>
>
