----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7909/#review13218 -----------------------------------------------------------
Overall an interesting patch. My main concern is the additional overhead of having to maintain our own messages, without it buying much. For example, whatever app receives one of these messages still needs Hive on its classpath to query the metastore for more details. Since that's the case, it would already have the thrift codegen to deserialize a Database/Table/Partition thrift message. If we could entirely ditch Hive from the classpath for receivers this totally makes sense, but I don't believe that's the case. A couple high-level comments about the patch itself: * This is a backwards-incompatible change in the release branch. Any reason not to implement this in trunk? Trunk has a lot of updates, and the patch will change significantly when ported over, so I'm thinking it would be best to do it there and perhaps start discussing a release, since its been a while and this problem keeps coming up. * In the message factory, we're making messages with a lot of setters, which opens us up to creating malformed messages since messages don't have the opportunity to sanity check themselves. If messages have all required fields, we should just pass them into the constructor. If there are lots of optional fields, perhaps a builder. Anything that lets the message sanity check that its valid before returning itself to the caller, since we can't later enforce they call a sanity check method. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/listener/NotificationListener.java <https://reviews.apache.org/r/7909/#comment28430> I think this indentation is off. Doesn't checkstyle complain about this? 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/#comment28434> Let's use the HiveConf keys as references, instead of strings. 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/#comment28431> Let's add a config option to choose which implementation to use. The default can be JSON. 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/#comment28432> Shouldn't this belong in the implementation, rather than the abstract class? 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/#comment28433> This belongs in the implementation, not the interface. 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/#comment28435> Any reason not to make this MessageUtil, and make all the methods static? Can you think of any non-static state this would need to keep? 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/#comment28436> What's the deal with having to construct the JSON yourself? I thought Jackson can do that for us, which would simplify things. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONMessageFactory.java <https://reviews.apache.org/r/7909/#comment28427> Can you import everything individually, instead of using star? Also, let's add a checkstyle rule to prevent star imports in the future. http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONMessageUtils.java <https://reviews.apache.org/r/7909/#comment28428> Package private? http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/java/org/apache/hcatalog/messaging/json/JSONMessageUtils.java <https://reviews.apache.org/r/7909/#comment28429> Does't jackson have methods to serialize objects into JSON automatically? Is there a good reason to do this ourselves? - Travis Crawford On Nov. 7, 2012, 1:28 a.m., Mithun Radhakrishnan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7909/ > ----------------------------------------------------------- > > (Updated Nov. 7, 2012, 1:28 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/java/org/apache/hcatalog/messaging/json/JSONMessageUtils.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/test/org/apache/hcatalog/listener/TestNotificationListener.java > 1406411 > > http://svn.apache.org/repos/asf/incubator/hcatalog/branches/branch-0.4/src/test/org/apache/hcatalog/messaging/json/TestJSONMessageDeserializer.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/7909/diff/ > > > Testing > ------- > > > Thanks, > > Mithun Radhakrishnan > >
