----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7909/#review13367 -----------------------------------------------------------
Overall this looks pretty good. I appreciate that you wrote javadocs too. Other than some smaller issues listed below, my main concern is putting this in the release branch before its in trunk. Despite our best intentions I've found priorities tend to shift when the immediate issue is resolved, and if we get this in trunk before the branch we're completely done now and don't need to revisit. Overall this looks pretty good though and I bet we could finish it up this week. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/MessageFactory.java <https://reviews.apache.org/r/7909/#comment28637> This should not be necessary if we create the default message factory with reflection. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/MessageFactory.java <https://reviews.apache.org/r/7909/#comment28635> I believe this should be initialized to null. Thanks for making this configurable, think this is some cruft from the previous patch. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/MessageFactory.java <https://reviews.apache.org/r/7909/#comment28636> Please put these constants in HCatConstants. We've been doing a good job of consolidating the "knobs" in one place, which makes it easier for people to understand what all can be configured. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/jms/MessageHelper.java <https://reviews.apache.org/r/7909/#comment28638> Can we rename this "MessagingUtil" and add a private constructor to prevent instantiation? This will ensure only static methods are added here. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONAddPartitionMessage.java <https://reviews.apache.org/r/7909/#comment28639> Thanks for making the messages self-check. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/test/org/apache/hcatalog/listener/TestNotificationListener.java <https://reviews.apache.org/r/7909/#comment28640> Please explicitly import. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/test/org/apache/hcatalog/listener/TestNotificationListener.java <https://reviews.apache.org/r/7909/#comment28641> Please explicitly import. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/test/org/apache/hcatalog/listener/TestNotificationListener.java <https://reviews.apache.org/r/7909/#comment28642> This test was actually refactored in trunk, switching to junit4-style tests. Take a look at: https://svn.apache.org/repos/asf/incubator/hcatalog/trunk/server-extensions/src/test/java/org/apache/hcatalog/listener/TestNotificationListener.java Can you backport that change too so the diff is smaller? Otherwise I could see accidental test coverage discrepancies between trunk/branches. - Travis Crawford On Nov. 9, 2012, 7:04 a.m., Mithun Radhakrishnan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7909/ > ----------------------------------------------------------- > > (Updated Nov. 9, 2012, 7:04 a.m.) > > > Review request for hcatalog, Alan Gates and Francis Liu. > > > Description > ------- > > In 0.4.1, the NotificationListener listens for metastore operations and emits > JMS notifications containing the entire metastore-objects > (Database/Table/Partitions) in Java-serialized form. The assumption at the > time was that consumers might need access to the whole object. This policy > poses a couple of problems: > 1. The notifications are verbose, since it conveys a bunch of information > that's available from querying the metastore anyway. > 2. Consumers of these JMS notifications (e.g. Oozie) would now be dependent > on the Java class definitions of metastore-objects. If they change, Oozie > would also need to be restarted (with updated libs), to consume the > notifications. > Ideally, the notifications should convey only the minimum information that > identifies the metastore-change unambiguously. (Everything else can be > queried for.) They should be backward compatible. If new fields are added, > existing consumers shouldn't break (unless they intend to consume the new > fields). Also, the notification-format ought to be pluggable. > For the initial rework, we're proposing to use a JSON-string to represent the > notification-content. We're also proposing a helper-class for the likes of > Oozie to use, that converts the strings to POJOs, in a backward-compatible > fashion. > > > This addresses bug HCATALOG-546. > https://issues.apache.org/jira/browse/HCATALOG-546 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/common/HCatConstants.java > 1406411 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/listener/NotificationListener.java > 1406411 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/AddPartitionMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/CreateDatabaseMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/CreateTableMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/DropDatabaseMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/DropPartitionMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/DropTableMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/HCatEventMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/MessageDeserializer.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/MessageFactory.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/jms/MessageHelper.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONAddPartitionMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONCreateDatabaseMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONCreateTableMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONDropDatabaseMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONDropPartitionMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONDropTableMessage.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONMessageDeserializer.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONMessageFactory.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/test/org/apache/hcatalog/listener/TestMsgBusConnection.java > 1406411 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/test/org/apache/hcatalog/listener/TestNotificationListener.java > 1406411 > > Diff: https://reviews.apache.org/r/7909/diff/ > > > Testing > ------- > > > Thanks, > > Mithun Radhakrishnan > >
