You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Patrik Márton via Review Board <no...@reviews.apache.org> on 2022/08/02 08:55:23 UTC
Review Request 74077: Refactor Atlas webapp module to remove Kafka core dependency
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74077/
-----------------------------------------------------------
Review request for atlas, Ashutosh Mestry, Jayendra Parab, and Nixon Rodrigues.
Bugs: ATLAS-4619
https://issues.apache.org/jira/browse/ATLAS-4619
Repository: atlas
Description
-------
Goal is to break the strong coupling between Atlas components and Kafka. These dependencies include using server side libraries of Kafka (this couples the Scala version and other non-public interfaces of Kafka). Any code using server side libraries of Kafka should be refactored.
The webapp module uses ShutdownAbleThread from the core kafka library. With this commit, it is changed to Thread, as the try-catch-finally block inside the run() method should solve the issue with closing the consumer.
Diffs
-----
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 49c504f9f
Diff: https://reviews.apache.org/r/74077/diff/1/
Testing
-------
- Unit tests are green
- Atlas server is healthy on manually provisoned cluster
- Tested the functionality with Hive Hook, no ConcurrentModificationException was present in the logs after stopping Atlas server.
Thanks,
Patrik Márton
Re: Review Request 74077: Refactor Atlas webapp module to remove Kafka core dependency
Posted by Ashutosh Mestry <as...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74077/#review224593
-----------------------------------------------------------
Ship it!
Ship It!
- Ashutosh Mestry
On Aug. 2, 2022, 1:21 p.m., Patrik Márton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74077/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2022, 1:21 p.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, and Nixon Rodrigues.
>
>
> Bugs: ATLAS-4619
> https://issues.apache.org/jira/browse/ATLAS-4619
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Goal is to break the strong coupling between Atlas components and Kafka. These dependencies include using server side libraries of Kafka (this couples the Scala version and other non-public interfaces of Kafka). Any code using server side libraries of Kafka should be refactored.
> The webapp module uses ShutdownAbleThread from the core kafka library. With this commit, it is changed to Thread, as the try-catch-finally block inside the run() method should solve the issue with closing the consumer.
>
>
> Diffs
> -----
>
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 49c504f9f
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java fdfc2560d
>
>
> Diff: https://reviews.apache.org/r/74077/diff/2/
>
>
> Testing
> -------
>
> - Unit tests are green
> - Atlas server is healthy on manually provisoned cluster
> - Tested the functionality with Hive Hook, no ConcurrentModificationException was present in the logs after stopping Atlas server.
>
>
> Thanks,
>
> Patrik Márton
>
>
Re: Review Request 74077: Refactor Atlas webapp module to remove Kafka core dependency
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74077/#review224601
-----------------------------------------------------------
Ship it!
Ship It!
- Madhan Neethiraj
On Aug. 2, 2022, 1:21 p.m., Patrik Márton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74077/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2022, 1:21 p.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, and Nixon Rodrigues.
>
>
> Bugs: ATLAS-4619
> https://issues.apache.org/jira/browse/ATLAS-4619
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Goal is to break the strong coupling between Atlas components and Kafka. These dependencies include using server side libraries of Kafka (this couples the Scala version and other non-public interfaces of Kafka). Any code using server side libraries of Kafka should be refactored.
> The webapp module uses ShutdownAbleThread from the core kafka library. With this commit, it is changed to Thread, as the try-catch-finally block inside the run() method should solve the issue with closing the consumer.
>
>
> Diffs
> -----
>
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 49c504f9f
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java fdfc2560d
>
>
> Diff: https://reviews.apache.org/r/74077/diff/2/
>
>
> Testing
> -------
>
> - Unit tests are green
> - Atlas server is healthy on manually provisoned cluster
> - Tested the functionality with Hive Hook, no ConcurrentModificationException was present in the logs after stopping Atlas server.
>
>
> Thanks,
>
> Patrik Márton
>
>
Re: Review Request 74077: Refactor Atlas webapp module to remove Kafka core dependency
Posted by Madhan Neethiraj <ma...@apache.org>.
> On Aug. 5, 2022, 12:30 a.m., Madhan Neethiraj wrote:
> > Patrik - the changes look good. Shouldn't these code changes result in webapp dependency updates to remove Kafka server libraries?
>
> Patrik Márton wrote:
> Thanks for the review! The dependency is coming transitively from the notification module as it uses EmbeddedKafkaServer. The plan is to remove this dependency from the notification module (by removing the EmbeddedKafkaServer), so it will not be transitively downloaded here. It is detalied in ATLAS-4620.
Thanks for the details. Looking forward to remove dependencies on Kafka server libraries (in ATLAS-4620).
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74077/#review224597
-----------------------------------------------------------
On Aug. 2, 2022, 1:21 p.m., Patrik Márton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74077/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2022, 1:21 p.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, and Nixon Rodrigues.
>
>
> Bugs: ATLAS-4619
> https://issues.apache.org/jira/browse/ATLAS-4619
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Goal is to break the strong coupling between Atlas components and Kafka. These dependencies include using server side libraries of Kafka (this couples the Scala version and other non-public interfaces of Kafka). Any code using server side libraries of Kafka should be refactored.
> The webapp module uses ShutdownAbleThread from the core kafka library. With this commit, it is changed to Thread, as the try-catch-finally block inside the run() method should solve the issue with closing the consumer.
>
>
> Diffs
> -----
>
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 49c504f9f
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java fdfc2560d
>
>
> Diff: https://reviews.apache.org/r/74077/diff/2/
>
>
> Testing
> -------
>
> - Unit tests are green
> - Atlas server is healthy on manually provisoned cluster
> - Tested the functionality with Hive Hook, no ConcurrentModificationException was present in the logs after stopping Atlas server.
>
>
> Thanks,
>
> Patrik Márton
>
>
Re: Review Request 74077: Refactor Atlas webapp module to remove Kafka core dependency
Posted by Patrik Márton via Review Board <no...@reviews.apache.org>.
> On Aug. 5, 2022, 12:30 a.m., Madhan Neethiraj wrote:
> > Patrik - the changes look good. Shouldn't these code changes result in webapp dependency updates to remove Kafka server libraries?
Thanks for the review! The dependency is coming transitively from the notification module as it uses EmbeddedKafkaServer. The plan is to remove this dependency from the notification module (by removing the EmbeddedKafkaServer), so it will not be transitively downloaded here. It is detalied in ATLAS-4620.
- Patrik
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74077/#review224597
-----------------------------------------------------------
On Aug. 2, 2022, 1:21 p.m., Patrik Márton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74077/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2022, 1:21 p.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, and Nixon Rodrigues.
>
>
> Bugs: ATLAS-4619
> https://issues.apache.org/jira/browse/ATLAS-4619
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Goal is to break the strong coupling between Atlas components and Kafka. These dependencies include using server side libraries of Kafka (this couples the Scala version and other non-public interfaces of Kafka). Any code using server side libraries of Kafka should be refactored.
> The webapp module uses ShutdownAbleThread from the core kafka library. With this commit, it is changed to Thread, as the try-catch-finally block inside the run() method should solve the issue with closing the consumer.
>
>
> Diffs
> -----
>
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 49c504f9f
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java fdfc2560d
>
>
> Diff: https://reviews.apache.org/r/74077/diff/2/
>
>
> Testing
> -------
>
> - Unit tests are green
> - Atlas server is healthy on manually provisoned cluster
> - Tested the functionality with Hive Hook, no ConcurrentModificationException was present in the logs after stopping Atlas server.
>
>
> Thanks,
>
> Patrik Márton
>
>
Re: Review Request 74077: Refactor Atlas webapp module to remove Kafka core dependency
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74077/#review224597
-----------------------------------------------------------
Patrik - the changes look good. Shouldn't these code changes result in webapp dependency updates to remove Kafka server libraries?
- Madhan Neethiraj
On Aug. 2, 2022, 1:21 p.m., Patrik Márton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74077/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2022, 1:21 p.m.)
>
>
> Review request for atlas, Ashutosh Mestry, Jayendra Parab, and Nixon Rodrigues.
>
>
> Bugs: ATLAS-4619
> https://issues.apache.org/jira/browse/ATLAS-4619
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Goal is to break the strong coupling between Atlas components and Kafka. These dependencies include using server side libraries of Kafka (this couples the Scala version and other non-public interfaces of Kafka). Any code using server side libraries of Kafka should be refactored.
> The webapp module uses ShutdownAbleThread from the core kafka library. With this commit, it is changed to Thread, as the try-catch-finally block inside the run() method should solve the issue with closing the consumer.
>
>
> Diffs
> -----
>
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 49c504f9f
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java fdfc2560d
>
>
> Diff: https://reviews.apache.org/r/74077/diff/2/
>
>
> Testing
> -------
>
> - Unit tests are green
> - Atlas server is healthy on manually provisoned cluster
> - Tested the functionality with Hive Hook, no ConcurrentModificationException was present in the logs after stopping Atlas server.
>
>
> Thanks,
>
> Patrik Márton
>
>
Re: Review Request 74077: Refactor Atlas webapp module to remove Kafka core dependency
Posted by Patrik Márton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74077/
-----------------------------------------------------------
(Updated Aug. 2, 2022, 1:21 p.m.)
Review request for atlas, Ashutosh Mestry, Jayendra Parab, and Nixon Rodrigues.
Changes
-------
Updated diff with some refactoring and increased timeout for Kafka consumer shutdown.
Bugs: ATLAS-4619
https://issues.apache.org/jira/browse/ATLAS-4619
Repository: atlas
Description
-------
Goal is to break the strong coupling between Atlas components and Kafka. These dependencies include using server side libraries of Kafka (this couples the Scala version and other non-public interfaces of Kafka). Any code using server side libraries of Kafka should be refactored.
The webapp module uses ShutdownAbleThread from the core kafka library. With this commit, it is changed to Thread, as the try-catch-finally block inside the run() method should solve the issue with closing the consumer.
Diffs (updated)
-----
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 49c504f9f
webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java fdfc2560d
Diff: https://reviews.apache.org/r/74077/diff/2/
Changes: https://reviews.apache.org/r/74077/diff/1-2/
Testing
-------
- Unit tests are green
- Atlas server is healthy on manually provisoned cluster
- Tested the functionality with Hive Hook, no ConcurrentModificationException was present in the logs after stopping Atlas server.
Thanks,
Patrik Márton