Return-Path: Delivered-To: apmail-hive-dev-archive@www.apache.org Received: (qmail 38434 invoked from network); 12 Apr 2011 16:17:55 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.3) by minotaur.apache.org with SMTP; 12 Apr 2011 16:17:55 -0000 Received: (qmail 41036 invoked by uid 500); 12 Apr 2011 16:17:55 -0000 Delivered-To: apmail-hive-dev-archive@hive.apache.org Received: (qmail 40939 invoked by uid 500); 12 Apr 2011 16:17:55 -0000 Mailing-List: contact dev-help@hive.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@hive.apache.org Delivered-To: mailing list dev@hive.apache.org Delivered-To: moderator for dev@hive.apache.org Received: (qmail 76416 invoked by uid 99); 12 Apr 2011 15:44:23 -0000 Content-Type: multipart/alternative; boundary="===============6229329116784596028==" MIME-Version: 1.0 Subject: Re: Review Request: Review request for HIVE-2038 From: "Ashutosh Chauhan" To: "John Sichi" , "Paul Yang" , "Carl Steinbach" Date: Tue, 12 Apr 2011 15:44:28 -0000 Message-ID: <20110412154428.9085.41182@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org X-ReviewRequest-URL: https://reviews.apache.org/r/581/ Cc: "Ashutosh Chauhan" ,"hive" In-Reply-To: <20110412030754.9085.56549@reviews.apache.org> References: <20110412030754.9085.56549@reviews.apache.org> --===============6229329116784596028== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On 2011-04-12 03:07:54, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.j= ava, line 999 > > > > > > Unrelated bugfix? Related bugfix, I will say : ) Without it, when drop partition returns from= object store, partition object doesn't contain partition values. = > On 2011-04-12 03:07:54, Carl Steinbach wrote: > > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 1= 80 > > > > > > Please add this property to hive-default.xml along with a descripti= on of what it does. > > Will add it in hive-default.xml. > On 2011-04-12 03:07:54, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore= .java, line 955 > > > > > > Please run checkstyle and correct any violations included in your p= atch. Will run checkstyle to check for any style violations. > On 2011-04-12 03:07:54, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreList= ener.java, line 27 > > > > > > Please add some javadoc explaining the intended use of this interfa= ce. = > > = > > * Are the methods called before or after an action completes? What = happens if a metastore operation fails? > > = > > * Are the methods allowed to block? Are they run in a separate thre= ad? > > = > > * Are the methods allowed to modify the catalog objects that are pa= ssed in as parameters? Will also add in javadoc. = * Methods are called after action completes. Only if action succeeds. They= are not called if operation fails since in that case nothing has actually = changed in metastore. * This is upto implementation. They can run in same thread, or they can sc= hedule there work in separate thread and return immediately. = * I don't see a reason to disallow modification of passed in parameter obj= ects. But, its mostly irrelevant here since methods are called after change= has already been persisted on metastore. So, modifying these objects can't= change any state on metastore. = > On 2011-04-12 03:07:54, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreList= ener.java, line 29 > > > > > > Instead of passing in raw Table/Partition/Database objects it may b= e better to instead wrap these objects in containers, e.g. CreateTableEvent= , DropTableEvent, etc. Eventually this interface will probably include onAl= terTable() and onAlterPartition(), and programmers will probably want to ac= cess both the before and after versions of a Table/Partition, etc. Whats the advantage of wrapper container objects? > On 2011-04-12 03:07:54, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore= .java, line 1446 > > > > > > No need to reference "this", right? Right. Though, I think using "this" in such cases improves code readability= . = > On 2011-04-12 03:07:54, Carl Steinbach wrote: > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreList= ener.java, line 26 > > > > > > What do you think about changing the name to MetaStoreEventListener= or CatalogEventListener? MetaStoreEventListener is fine too. - Ashutosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/581/#review428 ----------------------------------------------------------- On 2011-04-12 01:29:41, Ashutosh Chauhan wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/581/ > ----------------------------------------------------------- > = > (Updated 2011-04-12 01:29:41) > = > = > Review request for hive, Carl Steinbach, John Sichi, and Paul Yang. > = > = > Summary > ------- > = > Review request for HIVE-2038 > = > = > This addresses bug HIVE-2038. > https://issues.apache.org/jira/browse/HIVE-2038 > = > = > Diffs > ----- > = > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1079575 = > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore= .java 1079575 = > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreList= ener.java PRE-CREATION = > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/NoOpListener.= java PRE-CREATION = > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.j= ava 1079575 = > trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener= .java PRE-CREATION = > trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStore= Listener.java PRE-CREATION = > = > Diff: https://reviews.apache.org/r/581/diff > = > = > Testing > ------- > = > Unit test with dummy listener included. Also tested with an alternate lis= tener which sends a message on a message bus. > = > = > Thanks, > = > Ashutosh > = > --===============6229329116784596028==--