atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tom Beerbower <tbeerbo...@hortonworks.com>
Subject Re: Review Request 45720: Introduce Versioning to Atlas Notification Payload
Date Mon, 25 Apr 2016 13:12:00 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message