Re: Review Request: HIVE-2215

[John Sichi]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/883/#review822
---



trunk/metastore/src/model/package.jdo


Does indexing actually work on a LONGVARCHAR field across all DB's of 
interest?


- John


On 2011-06-10 21:24:13, Ashutosh Chauhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/883/
> ---
> 
> (Updated 2011-06-10 21:24:13)
> 
> 
> Review request for hive and John Sichi.
> 
> 
> Summary
> ---
> 
> Follow-up for HIVE-2147.
> 
> 
> This addresses bug HIVE-2215.
> https://issues.apache.org/jira/browse/HIVE-2215
> 
> 
> Diffs
> -
> 
>   trunk/metastore/if/hive_metastore.thrift 1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
>  1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
>  1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
>  1134443 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> 1134443 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> 1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/events/MarkPartitionEvent.java
>  PRE-CREATION 
>   
> trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MPartitionEvent.java
>  PRE-CREATION 
>   trunk/metastore/src/model/package.jdo 1134443 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java 
> 1134443 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMarkPartitionSet.java
>  PRE-CREATION 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
>  1134443 
> 
> Diff: https://reviews.apache.org/r/883/diff
> 
> 
> Testing
> ---
> 
> Added test cases for new api.
> 
> 
> Thanks,
> 
> Ashutosh
> 
>

Re: Review Request: HIVE-2215

[Ashutosh Chauhan]

> On 2011-06-13 21:47:25, John Sichi wrote:
> > trunk/metastore/src/model/package.jdo, line 670
> > 
> >
> > Does indexing actually work on a LONGVARCHAR field across all DB's of 
> > interest?

No, it doesn't. So, I reverted it back to VARCHAR.

If the rest of the patch looks alright, I will attach a new patch with this 
change.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/883/#review822
---


On 2011-06-10 21:24:13, Ashutosh Chauhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/883/
> ---
> 
> (Updated 2011-06-10 21:24:13)
> 
> 
> Review request for hive and John Sichi.
> 
> 
> Summary
> ---
> 
> Follow-up for HIVE-2147.
> 
> 
> This addresses bug HIVE-2215.
> https://issues.apache.org/jira/browse/HIVE-2215
> 
> 
> Diffs
> -
> 
>   trunk/metastore/if/hive_metastore.thrift 1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
>  1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
>  1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
>  1134443 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> 1134443 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> 1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/events/MarkPartitionEvent.java
>  PRE-CREATION 
>   
> trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MPartitionEvent.java
>  PRE-CREATION 
>   trunk/metastore/src/model/package.jdo 1134443 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java 
> 1134443 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMarkPartitionSet.java
>  PRE-CREATION 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
>  1134443 
> 
> Diff: https://reviews.apache.org/r/883/diff
> 
> 
> Testing
> ---
> 
> Added test cases for new api.
> 
> 
> Thanks,
> 
> Ashutosh
> 
>

Re: Review Request: HIVE-2215

[Carl Steinbach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/883/#review824
---



trunk/metastore/if/hive_metastore.thrift


I think this should be changed to "PartitionEventType" in order to make it 
clear that this applies to partitions only. If in the future we need to 
introduce event types for tables, indexes, etc, then we should add new enums 
for those event types as well.



trunk/metastore/if/hive_metastore.thrift


This should also throw UnknownDBException and UnknownTableException. The 
same goes for isPartitionMarkedForEvent.



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java


Collections aren't required to satisfy an ordering property, so we have to 
assume the output of this logging statement is ambiguous, e.g. "[a, b]" versus 
"[b, a]". We should disambiguate this by passing in the part_vals map and 
logging the key/value pairs instead of just the values.



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java


Missing exceptions: UnknownDbException and UnknownTableException.



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java


Checking to see if the DB and Table exist should be done in the same 
database transaction as the rest of the operation. If you do it here there's no 
guarantee that the db/table will still exist when ms.markPartitionForEvent() is 
called.



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java


Should we add an InvalidPartitionException and UnknownPartitionException? 
Seems like those are both valid exceptions in this situation.



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java


Same issue here as before. These checks need to get pushed into 
ms.isPartitionMarkedForEvent().



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java


I think the name of this method is misleading. You're marking a single 
partition done, not a set of partitions, right? 

Also, in this context being "done" means that the load operation on that 
partition has completed, so it would be good to include "load" in the name of 
the method and event class, e.g. "LoadPartitionDoneEvent" and 
"onLoadPartitionDone".




trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MPartitionEvent.java


Is it possible to use org.apache.hadoop.hive.metastore.api.EventType 
instead of int?

Another approach is to create an MPartitionEvent baseclass, and then 
subclass that with MPartitionLoadDoneEvent, etc, and use eventType as the 
internal type discriminator for JDO.



trunk/metastore/src/model/package.jdo


You need to supply schema upgrade scripts for Derby and MySQL. Please 
either do that in this ticket or open a followup ticket and assign it to 
yourself.



trunk/metastore/src/model/package.jdo


It looks like it's possible for this table to hold more than one 
"MarkPartitionDone" event for the same partition, but is that a legal state? If 
it is, how do you know when the load operation for a partition is still in 
progress?



trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMarkPartitionSet.java


Can you subclass this with a remote and embedded version?



trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java


Any reason in particular why you switched to always running this test in 
local mode? If we can only test one scenario, then I think there's more value 
in focusing on the standalone client/server setup.


- Carl


On 2011-06-10 21:24:13, Ashutosh Chauhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/883/
> ---
> 
> (Updated 2011-06-10 21:24:13)
> 
> 
> Review request for hive and John Sichi.
> 
> 
> Summary
> ---
> 
> Follow-up for HIVE-2147.
> 
> 
> This addresses bug HIVE-2215.
> https://issues.apache.org/jira/browse/HIVE-2215
> 
> 
> Diffs
> -
> 
>   trunk/metastore/if/hive_metastore.thrift 1134443 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metasto

Re: Review Request: HIVE-2215

[Ashutosh Chauhan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/883/
---

(Updated 2011-06-14 20:51:53.179968)


Review request for hive and John Sichi.


Changes
---

Updated patch with Carl's comments.
Carl, can you take a look?


Summary
---

Follow-up for HIVE-2147.


This addresses bug HIVE-2215.
https://issues.apache.org/jira/browse/HIVE-2215


Diffs (updated)
-

  trunk/metastore/if/hive_metastore.thrift 1135779 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
1135779 
  
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
 1135779 
  
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 
1135779 
  
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
 1135779 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
1135779 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
1135779 
  
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/events/LoadPartitionDoneEvent.java
 PRE-CREATION 
  
trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MPartitionEvent.java
 PRE-CREATION 
  trunk/metastore/src/model/package.jdo 1135779 
  trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java 
1135779 
  
trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMarkPartition.java
 PRE-CREATION 
  
trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMarkPartitionRemote.java
 PRE-CREATION 
  
trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
 1135779 

Diff: https://reviews.apache.org/r/883/diff


Testing
---

Added test cases for new api.


Thanks,

Ashutosh

Re: Review Request: HIVE-2215

[Ashutosh Chauhan]

> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/if/hive_metastore.thrift, line 46
> > 
> >
> > I think this should be changed to "PartitionEventType" in order to make 
> > it clear that this applies to partitions only. If in the future we need to 
> > introduce event types for tables, indexes, etc, then we should add new 
> > enums for those event types as well.

Done.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/if/hive_metastore.thrift, line 338
> > 
> >
> > This should also throw UnknownDBException and UnknownTableException. 
> > The same goes for isPartitionMarkedForEvent.

Done.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
> >  line 528
> > 
> >
> > Collections aren't required to satisfy an ordering property, so we have 
> > to assume the output of this logging statement is ambiguous, e.g. "[a, b]" 
> > versus "[b, a]". We should disambiguate this by passing in the part_vals 
> > map and logging the key/value pairs instead of just the values.

Done.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
> >  line 3182
> > 
> >
> > Missing exceptions: UnknownDbException and UnknownTableException.

Done.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
> >  line 3186
> > 
> >
> > Checking to see if the DB and Table exist should be done in the same 
> > database transaction as the rest of the operation. If you do it here 
> > there's no guarantee that the db/table will still exist when 
> > ms.markPartitionForEvent() is called.

Done.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
> >  line 3188
> > 
> >
> > Should we add an InvalidPartitionException and 
> > UnknownPartitionException? Seems like those are both valid exceptions in 
> > this situation.

Done.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
> >  line 3224
> > 
> >
> > Same issue here as before. These checks need to get pushed into 
> > ms.isPartitionMarkedForEvent().

Done.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java,
> >  line 82
> > 
> >
> > I think the name of this method is misleading. You're marking a single 
> > partition done, not a set of partitions, right? 
> > 
> > Also, in this context being "done" means that the load operation on 
> > that partition has completed, so it would be good to include "load" in the 
> > name of the method and event class, e.g. "LoadPartitionDoneEvent" and 
> > "onLoadPartitionDone".
> >

Done.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MPartitionEvent.java,
> >  line 34
> > 
> >
> > Is it possible to use org.apache.hadoop.hive.metastore.api.EventType 
> > instead of int?
> > 
> > Another approach is to create an MPartitionEvent baseclass, and then 
> > subclass that with MPartitionLoadDoneEvent, etc, and use eventType as the 
> > internal type discriminator for JDO.

No.

I don't see any benefit of it. Nor I cant see how will this work.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/src/model/package.jdo, line 668
> > 
> >
> > You need to supply schema upgrade scripts for Derby and MySQL. Please 
> > either do that in this ticket or open a followup ticket and assign it to 
> > yourself.

Will do.


> On 2011-06-14 01:02:20, Carl Steinbach wrote:
> > trunk/metastore/src/model/package.jdo, line 683
> > 
> >
> > It looks like it's possible for this table to hold more than one 
> > "MarkPartitionDone" event for the same partition, but is that a legal 
> > state? If it is, how do you know when the load operation for a partition is 
> > still in progress?

This is not for when load operation is in progress. As suggested from name its 
"LoadPartiti

Re: Review Request: HIVE-2215

[Carl Steinbach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/883/#review843
---

Ship it!


Looks good to me. Thanks for making the changes.

- Carl


On 2011-06-14 20:51:53, Ashutosh Chauhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/883/
> ---
> 
> (Updated 2011-06-14 20:51:53)
> 
> 
> Review request for hive and John Sichi.
> 
> 
> Summary
> ---
> 
> Follow-up for HIVE-2147.
> 
> 
> This addresses bug HIVE-2215.
> https://issues.apache.org/jira/browse/HIVE-2215
> 
> 
> Diffs
> -
> 
>   trunk/metastore/if/hive_metastore.thrift 1135779 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 1135779 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
>  1135779 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
>  1135779 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
>  1135779 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> 1135779 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> 1135779 
>   
> trunk/metastore/src/java/org/apache/hadoop/hive/metastore/events/LoadPartitionDoneEvent.java
>  PRE-CREATION 
>   
> trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MPartitionEvent.java
>  PRE-CREATION 
>   trunk/metastore/src/model/package.jdo 1135779 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java 
> 1135779 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMarkPartition.java
>  PRE-CREATION 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMarkPartitionRemote.java
>  PRE-CREATION 
>   
> trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
>  1135779 
> 
> Diff: https://reviews.apache.org/r/883/diff
> 
> 
> Testing
> ---
> 
> Added test cases for new api.
> 
> 
> Thanks,
> 
> Ashutosh
> 
>