atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hemanth Yamijala <yhema...@gmail.com>
Subject Re: Review Request 45720: Introduce Versioning to Atlas Notification Payload
Date Thu, 07 Apr 2016 02:24:21 GMT


> On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote:
> > notification/src/main/java/org/apache/atlas/notification/MessageVersion.java, line
84
> > <https://reviews.apache.org/r/45720/diff/2/?file=1325393#file1325393line84>
> >
> >     May not be a case that actually happens, but would we treat 1.0 and 1 as the
same version? Should we handle that?
> 
> Tom Beerbower wrote:
>     It should be the case with the patch that 1.0 equals 1 (See the unit test in MessageVersionTest).
 Are you saying that they should be different versions?

The equals check is happening on the String created like this new MessageVersion("1.0.0").
So, if someone creates a version like this: new MessageVersion("1.0"), the string comparison
would equal to false, no?


> On April 6, 2016, 2:05 p.m., Hemanth Yamijala wrote:
> > notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java,
line 95
> > <https://reviews.apache.org/r/45720/diff/2/?file=1325396#file1325396line95>
> >
> >     This exception seems to be stopping the message consuming thread. That would
be a problem.
> 
> Tom Beerbower wrote:
>     I think that in the HookConsumer runnable (in NotificationHookConsumer) that it catches
Throwable and just logs a warning.
>     
>     Are you saying that throwing a runtime exception here is a bad idea?  If so, what
would you say is the best way to handle a version mismatch error during a consumer.next()
call?  I see three posibilities ...
>     
>     1) throw a runtime exception.  Means that the calling code needs to handle the exception.
>     2) throw a checked exception.  Means that we need to change the signature of our
consumer interface and that any existing users need to change their code to handle the exception.
>     3) return null.  Means that existing users must change their code to handle possible
null return or suffer NPE.

Sorry - I didn't notice the outer loop. This is fine.


- Hemanth


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45720/#review127308
-----------------------------------------------------------


On April 6, 2016, 6:17 p.m., Tom Beerbower wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45720/
> -----------------------------------------------------------
> 
> (Updated April 6, 2016, 6:17 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
11b7a53 
>   typesystem/src/main/resources/atlas-log4j.xml 2bb49d3 
> 
> Diff: https://reviews.apache.org/r/45720/diff/
> 
> 
> Testing
> -------
> 
> New unit tests added.
> 
> mvn clean test.  
> 
> Additional testing in progress.
> 
> 
> Thanks,
> 
> Tom Beerbower
> 
>


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