----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62681/#review186624 -----------------------------------------------------------
notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java Lines 163 (patched) <https://reviews.apache.org/r/62681/#comment263337> Consider using a more efficient way to generating msgId strings. notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java Lines 168 (patched) <https://reviews.apache.org/r/62681/#comment263339> Is it possible that compressedMsg will be longer than msgJson? If so, will it be better to send uncompressed msgJson? notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java Lines 177 (patched) <https://reviews.apache.org/r/62681/#comment263338> Consider more efficient way of generating msgId string. notification/src/main/java/org/apache/atlas/notification/AlasNotificationMessageDeserializer.java Lines 36 (patched) <https://reviews.apache.org/r/62681/#comment263333> Please rename this class appropriately to AtlasNotificationMessageDeserializer. notification/src/main/java/org/apache/atlas/notification/AlasNotificationMessageDeserializer.java Lines 74 (patched) <https://reviews.apache.org/r/62681/#comment263336> Consider using a local variable to store store msgJson. Using formal argument to store changing value of msgJson is confusing to read. Is this API called in a thread-safe way? notification/src/main/java/org/apache/atlas/notification/AlasNotificationMessageDeserializer.java Lines 80 (patched) <https://reviews.apache.org/r/62681/#comment263334> Is there a specific test case for this? If not, please consider writing one. notification/src/main/java/org/apache/atlas/notification/AlasNotificationMessageDeserializer.java Lines 90 (patched) <https://reviews.apache.org/r/62681/#comment263335> Please consider logging this as ERROR. This should never happen! notification/src/main/java/org/apache/atlas/notification/AlasNotificationMessageDeserializer.java Lines 138 (patched) <https://reviews.apache.org/r/62681/#comment263340> Consider adding another validation to ensure that the msgStr's size is equal to or less than MESSAGE_MAX_LENGTH. - Abhay Kulkarni On Sept. 29, 2017, 12:26 a.m., Madhan Neethiraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62681/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2017, 12:26 a.m.) > > > Review request for atlas, Apoorv Naik, Ashutosh Mestry, Abhay Kulkarni, > Sarath Subramanian, and Suma Shivaprasad. > > > Bugs: ATLAS-2075 > https://issues.apache.org/jira/browse/ATLAS-2075 > > > Repository: atlas > > > Description > ------- > > Update notification module to compress and split large messages (> 1mb) that > can't be sent via Kafka. When such a message is sent, the message is first > compressed via GZip. If compressed message is within the size limit, the > message is sent via Kafka. If the compressed size is larger, then the message > is split into multiple small messages and sent via Kafka. > > Notification consumer is updated to merge and uncompress messages before the > message is processed further. > > > Diffs > ----- > > common/src/main/java/org/apache/atlas/AtlasConfiguration.java 9a9bb76a > notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java > d3b4e49e > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java > 38889ef8 > > notification/src/main/java/org/apache/atlas/notification/AbstractMessageDeserializer.java > ec99372d > > notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java > cb44fc68 > > notification/src/main/java/org/apache/atlas/notification/AlasNotificationMessageDeserializer.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/AtlasNotificationBaseMessage.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/AtlasNotificationStringMessage.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/MessageVersion.java > 6ef407ac > > notification/src/main/java/org/apache/atlas/notification/NotificationInterface.java > 956c85e0 > > notification/src/main/java/org/apache/atlas/notification/VersionedMessage.java > 1929eb46 > > notification/src/main/java/org/apache/atlas/notification/VersionedMessageDeserializer.java > cc2099e7 > notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java > 9b712f44 > > notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationMockTest.java > b7474a0e > > notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java > 3b2a093b > > notification/src/test/java/org/apache/atlas/notification/AbstractNotificationTest.java > 61107a9f > > notification/src/test/java/org/apache/atlas/notification/VersionedMessageTest.java > 587b7ebd > > notification/src/test/java/org/apache/atlas/notification/entity/EntityMessageDeserializerTest.java > be324277 > > notification/src/test/java/org/apache/atlas/notification/hook/HookMessageDeserializerTest.java > 3724fd5f > > > Diff: https://reviews.apache.org/r/62681/diff/2/ > > > Testing > ------- > > - verified that large messages are compressed and split by Atlas hook, before > sending to Kafka > - verified that Atlas server processes compressed, multi-part messages > correctly > > > Thanks, > > Madhan Neethiraj > >