> On 2011-06-14 01:02:20, Carl Steinbach wrote: > > trunk/metastore/if/hive_metastore.thrift, line 46 > > <https://reviews.apache.org/r/883/diff/1/?file=20969#file20969line46> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20969#file20969line338> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20970#file20970line528> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20970#file20970line3182> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20970#file20970line3186> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20970#file20970line3188> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20970#file20970line3224> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20973#file20973line82> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20977#file20977line34> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20978#file20978line668> > > > > 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 > > <https://reviews.apache.org/r/883/diff/1/?file=20978#file20978line683> > > > > 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 "LoadPartitionDone". So, marking partition load done is idempotent. Client can mark it multiple times. So, metastore will return true if it finds one or more such partitioned marked in the table. > On 2011-06-14 01:02:20, Carl Steinbach wrote: > > trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMarkPartitionSet.java, > > line 36 > > <https://reviews.apache.org/r/883/diff/1/?file=20980#file20980line36> > > > > Can you subclass this with a remote and embedded version? Done. > On 2011-06-14 01:02:20, Carl Steinbach wrote: > > trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java, > > line 80 > > <https://reviews.apache.org/r/883/diff/1/?file=20981#file20981line80> > > > > 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. I reverted those changes. - Ashutosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/883/#review824 ----------------------------------------------------------- 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 > >
