> On Nov. 7, 2012, 7:31 p.m., Travis Crawford wrote: > > 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.
I'll port this to trunk as well, but we're looking integrate Oozie with HCatalog 0.4.2. Also, I've changed the serialization-code to have Jackson do it. I'll post the modified patch shortly. Regarding using Thrift to serialize, I'll post my reasoning on the JIRA. :] Thank you, Travis. :] - Mithun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7909/#review13218 ----------------------------------------------------------- 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 > >
