You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Ashutosh Mestry via Review Board <no...@reviews.apache.org> on 2021/02/15 05:17:33 UTC
Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/
-----------------------------------------------------------
Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
Bugs: ATLAS-4155
https://issues.apache.org/jira/browse/ATLAS-4155
Repository: atlas
Description
-------
**Approach**
- Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
- Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
Diffs
-----
notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
Diff: https://reviews.apache.org/r/73185/diff/1/
Testing
-------
**PC Build**
https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
**Functional tests**
Large message payload.
Thanks,
Ashutosh Mestry
Re: Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/#review222593
-----------------------------------------------------------
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Line 589 (original), 590 (patched)
<https://reviews.apache.org/r/73185/#comment311705>
is 'failedCommitOffsetRecorder' any more needed? Please review. If it is not needed, please remove.
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Line 926 (original), 927 (patched)
<https://reviews.apache.org/r/73185/#comment311706>
I recently saw a deployment using multiple partitions on a topic. To handle such configurations, consider replacing 'long lastCommittedOffset' with:
Map<TopicPartition, OffsetAndMetadata> lastCommittedOffset;
And send this to AtlasKafkaConsumer.receiveWithCheckedCommit() so that it can get topic/partition specifc offset and commit when appropriate.
- Madhan Neethiraj
On Feb. 15, 2021, 5:17 a.m., Ashutosh Mestry wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73185/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2021, 5:17 a.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4155
> https://issues.apache.org/jira/browse/ATLAS-4155
>
>
> Repository: atlas
>
>
> Description
> -------
>
> **Approach**
> - Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
> - Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
>
>
> Diffs
> -----
>
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
>
>
> Diff: https://reviews.apache.org/r/73185/diff/1/
>
>
> Testing
> -------
>
> **PC Build**
> https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
>
> **Functional tests**
> Large message payload.
>
>
> Thanks,
>
> Ashutosh Mestry
>
>
Re: Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/#review222601
-----------------------------------------------------------
notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java
Lines 116 (patched)
<https://reviews.apache.org/r/73185/#comment311709>
records with offset less than the committed offset should be skipped right? This seems to skip records with offset higher than the committed offset. Please review and update.
- Madhan Neethiraj
On Feb. 17, 2021, 7:03 a.m., Ashutosh Mestry wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73185/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2021, 7:03 a.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4155
> https://issues.apache.org/jira/browse/ATLAS-4155
>
>
> Repository: atlas
>
>
> Description
> -------
>
> **Approach**
> - Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
> - Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
>
>
> Diffs
> -----
>
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 33191a7a6
>
>
> Diff: https://reviews.apache.org/r/73185/diff/2/
>
>
> Testing
> -------
>
> **PC Build**
> https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
>
> **Functional tests**
> Large message payload.
>
>
> Thanks,
>
> Ashutosh Mestry
>
>
Re: Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/#review222604
-----------------------------------------------------------
Fix it, then Ship it!
notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java
Lines 118 (patched)
<https://reviews.apache.org/r/73185/#comment311714>
Consider logging following:
LOG.info("Skipping already processed message: topic={}, partition={} offset={}. Last processed offset={}", record.topic(), record.partition(), record.offset(), lastCommittedPartitionOffset.get(topicPartition));
notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java
Lines 230 (patched)
<https://reviews.apache.org/r/73185/#comment311713>
instead of returning null, perhaps return should be:
return this.receive();
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Lines 193 (patched)
<https://reviews.apache.org/r/73185/#comment311712>
Consider marking lastCommittedPartitionOffset as final.
- Madhan Neethiraj
On Feb. 17, 2021, 6:19 p.m., Ashutosh Mestry wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73185/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2021, 6:19 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4155
> https://issues.apache.org/jira/browse/ATLAS-4155
>
>
> Repository: atlas
>
>
> Description
> -------
>
> **Approach**
> - Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
> - Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
>
>
> Diffs
> -----
>
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 33191a7a6
>
>
> Diff: https://reviews.apache.org/r/73185/diff/3/
>
>
> Testing
> -------
>
> **PC Build**
> https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
>
> **Functional tests**
> Large message payload.
>
>
> Thanks,
>
> Ashutosh Mestry
>
>
Re: Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/#review222605
-----------------------------------------------------------
Ship it!
Ship It!
- Madhan Neethiraj
On Feb. 17, 2021, 7:12 p.m., Ashutosh Mestry wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73185/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2021, 7:12 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4155
> https://issues.apache.org/jira/browse/ATLAS-4155
>
>
> Repository: atlas
>
>
> Description
> -------
>
> **Approach**
> - Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
> - Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
>
>
> Diffs
> -----
>
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 33191a7a6
>
>
> Diff: https://reviews.apache.org/r/73185/diff/4/
>
>
> Testing
> -------
>
> **PC Build**
> https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
> https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/406/
>
> **Functional tests**
> Large message payload.
>
>
> Thanks,
>
> Ashutosh Mestry
>
>
Re: Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
Posted by Ashutosh Mestry via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/
-----------------------------------------------------------
(Updated Feb. 18, 2021, 5:19 a.m.)
Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
Changes
-------
Updates include: Addressed review comments.
Bugs: ATLAS-4155
https://issues.apache.org/jira/browse/ATLAS-4155
Repository: atlas
Description
-------
**Approach**
- Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
- Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
Diffs (updated)
-----
notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 33191a7a6
Diff: https://reviews.apache.org/r/73185/diff/5/
Changes: https://reviews.apache.org/r/73185/diff/4-5/
Testing
-------
**PC Build**
https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/406/
**Functional tests**
Large message payload.
Thanks,
Ashutosh Mestry
Re: Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
Posted by Sarath Subramanian <sa...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/#review222606
-----------------------------------------------------------
Fix it, then Ship it!
notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java
Lines 23 (patched)
<https://reviews.apache.org/r/73185/#comment311717>
nit: remove unused import
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Line 927 (original), 920 (patched)
<https://reviews.apache.org/r/73185/#comment311716>
consider adding to 'lastCommittedPartitionOffset' after successful commit.
=> move line 920 after 921.
- Sarath Subramanian
On Feb. 17, 2021, 11:12 a.m., Ashutosh Mestry wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73185/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2021, 11:12 a.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4155
> https://issues.apache.org/jira/browse/ATLAS-4155
>
>
> Repository: atlas
>
>
> Description
> -------
>
> **Approach**
> - Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
> - Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
>
>
> Diffs
> -----
>
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 33191a7a6
>
>
> Diff: https://reviews.apache.org/r/73185/diff/4/
>
>
> Testing
> -------
>
> **PC Build**
> https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
> https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/406/
>
> **Functional tests**
> Large message payload.
>
>
> Thanks,
>
> Ashutosh Mestry
>
>
Re: Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
Posted by Ashutosh Mestry via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/
-----------------------------------------------------------
(Updated Feb. 17, 2021, 7:12 p.m.)
Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
Changes
-------
Updates include: Addressed review comments.
Bugs: ATLAS-4155
https://issues.apache.org/jira/browse/ATLAS-4155
Repository: atlas
Description
-------
**Approach**
- Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
- Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
Diffs (updated)
-----
notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 33191a7a6
Diff: https://reviews.apache.org/r/73185/diff/4/
Changes: https://reviews.apache.org/r/73185/diff/3-4/
Testing
-------
**PC Build**
https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
**Functional tests**
Large message payload.
Thanks,
Ashutosh Mestry
Re: Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
Posted by Ashutosh Mestry via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/
-----------------------------------------------------------
(Updated Feb. 17, 2021, 6:19 p.m.)
Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
Changes
-------
Updates include: Addressed review comments.
Bugs: ATLAS-4155
https://issues.apache.org/jira/browse/ATLAS-4155
Repository: atlas
Description
-------
**Approach**
- Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
- Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
Diffs (updated)
-----
notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 33191a7a6
Diff: https://reviews.apache.org/r/73185/diff/3/
Changes: https://reviews.apache.org/r/73185/diff/2-3/
Testing
-------
**PC Build**
https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
**Functional tests**
Large message payload.
Thanks,
Ashutosh Mestry
Re: Review Request 73185: NotificationHookConsumer: Large Compressed
Message Processing Problem
Posted by Ashutosh Mestry via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73185/
-----------------------------------------------------------
(Updated Feb. 17, 2021, 7:03 a.m.)
Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
Changes
-------
Updates include: Addressed review comments.
Bugs: ATLAS-4155
https://issues.apache.org/jira/browse/ATLAS-4155
Repository: atlas
Description
-------
**Approach**
- Modify: _NotificaitonHookConsumer_: Add lastOffsetToCommit. Update this on every commit called. Call the new method on _AtlasKafkaConsumer.receiveWithCheckedCommit_
- Modify: _AtlasKafkaConsumer_: Add new method _receiveWithCheckedCommit_.
Diffs (updated)
-----
notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java c38a504fa
notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java f3e81ecc2
notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 05d0d8137
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0e58dacff
webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 33191a7a6
Diff: https://reviews.apache.org/r/73185/diff/2/
Changes: https://reviews.apache.org/r/73185/diff/1-2/
Testing
-------
**PC Build**
https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/392/
**Functional tests**
Large message payload.
Thanks,
Ashutosh Mestry