> On April 25, 2016, 7:10 a.m., Shwetha GS wrote: > > distro/src/conf/atlas-log4j.xml, line 57 > > <https://reviews.apache.org/r/45720/diff/6/?file=1353178#file1353178line57> > > > > These logs are created at client space and we shouldn't force them to > > create these loggers. Lets use default loggers. > > > > For any incompatible messages in hook notifications, we can use this > > logger(after catching the IncompatibleVersionException as atlas is the > > client in this case)
Could you explain more what the concern is? There is a requirement to log the incompatible messages to a different file. > On April 25, 2016, 7:10 a.m., Shwetha GS wrote: > > notification/src/main/java/org/apache/atlas/notification/MessageVersion.java, > > line 35 > > <https://reviews.apache.org/r/45720/diff/6/?file=1353186#file1353186line35> > > > > Lets just compute everytime and remove synchronized on getVersionParts() I went back and forth on this. Since the version is final, I didn't want to keep executing the same code over and over just to produce the same result. In reality, getVersionParts isn't going to get called that much so it probably doesn't really matter either way. Other than making the code slightly cleaner, is there some other reason why? > On April 25, 2016, 7:10 a.m., Shwetha GS wrote: > > notification/src/main/java/org/apache/atlas/notification/MessageVersion.java, > > line 85 > > <https://reviews.apache.org/r/45720/diff/6/?file=1353186#file1353186line85> > > > > Use compareTo() == 0 This is another thing that I went back and forth on ... Just remember that compareTo takes a MessageVersion while equals takes an Object as a parameter. I can't simply cast the Object to a MessageVersion in equals without checking or it could result in a ClassCastException. So I need to keep the code that checks the class type ... if (that == null || getClass() != that.getClass()) { return false; } Also, it makes sense to leave the == comparison because it's almost no cost and just good practice for equals, I think. I guess it still makes sense to call compareTo from equals to ensure they stay consistent. - Tom ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45720/#review130345 ----------------------------------------------------------- On April 20, 2016, 1:26 p.m., Tom Beerbower wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45720/ > ----------------------------------------------------------- > > (Updated April 20, 2016, 1:26 p.m.) > > > Review request for atlas. > > > Bugs: ATLAS-631 > https://issues.apache.org/jira/browse/ATLAS-631 > > > Repository: atlas > > > Description > ------- > > 1. Introduce Versioning to Atlas Notification Payload (both ways) > 2. For any messages that are not able to be processed, log the message do a > separate log file for unprocessed messages. > > > Diffs > ----- > > distro/src/conf/atlas-log4j.xml 1ac4082 > notification/src/main/java/org/apache/atlas/kafka/KafkaConsumer.java > 029a072 > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java > 889af11 > > notification/src/main/java/org/apache/atlas/notification/AbstractMessageDeserializer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java > 596f988 > > notification/src/main/java/org/apache/atlas/notification/AbstractNotificationConsumer.java > 1cadb99 > > notification/src/main/java/org/apache/atlas/notification/IncompatibleVersionException.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/MessageDeserializer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/MessageVersion.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/NotificationInterface.java > ac285aa > > notification/src/main/java/org/apache/atlas/notification/VersionedMessage.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/entity/EntityMessageDeserializer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/hook/HookMessageDeserializer.java > PRE-CREATION > notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java > db34815 > > notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/AbstractNotificationTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/MessageVersionTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/VersionedMessageTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/entity/EntityMessageDeserializerTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/entity/EntityNotificationImplTest.java > 385c41f > > notification/src/test/java/org/apache/atlas/notification/hook/HookMessageDeserializerTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/hook/HookNotificationTest.java > 57b0eea > typesystem/src/main/resources/atlas-log4j.xml 2bb49d3 > > Diff: https://reviews.apache.org/r/45720/diff/ > > > Testing > ------- > > New unit tests added. > > mvn clean test. > > > Thanks, > > Tom Beerbower > >
