You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Nixon Rodrigues <ni...@freestoneinfotech.com> on 2017/07/03 13:17:48 UTC
Review Request 60597: ATLAS-1908 : - Updating old Kafka consumer api
properties to reflect change in new KafkaConsumer configs for Atlas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60597/
-----------------------------------------------------------
Review request for atlas, Apoorv Naik, Madhan Neethiraj, and Sarath Subramanian.
Bugs: ATLAS-1908
https://issues.apache.org/jira/browse/ATLAS-1908
Repository: atlas
Description
-------
This patch handles old kafka consumer configs values to be added as new configs if new configs are not present , so that Kafka consumer starts gracefully even if new configs are not added explicitly.
eg. atlas.kafka.auto.commit.enable=false to atlas.kafka.enable.auto.commit=false
atlas.kafka.auto.offset.reset=smallest to atlas.kafka.auto.offset.reset=earliest
atlas.kafka.session.timeout.ms=30000
Also added poll.timeout.ms in configs so kafka consumer can be configurable.This involve change in AtlasKafkaConsumer receive interface.
Diffs
-----
distro/src/conf/atlas-application.properties 474f253
notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java 9c15243
notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 366c8a7
notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java 22e40f9
notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java 70059cb
notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java c791d43
notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 8324b57
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 9e5b864
webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 650ca0a
webapp/src/test/java/org/apache/atlas/web/integration/BaseResourceIT.java c036cfa
Diff: https://reviews.apache.org/r/60597/diff/1/
Testing
-------
Executed UT/IT's using mvn clean install.
Tested KafkaConsumer with Hive Hook by creating table in hive.
Atlas starts gracefully and Kafka consumer is up without error when new properties are not added.
Thanks,
Nixon Rodrigues
Re: Review Request 60597: ATLAS-1908 : - Updating old Kafka consumer
api
properties to reflect change in new KafkaConsumer configs for Atlas
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
> On July 13, 2017, 5:03 a.m., Ashutosh Mestry wrote:
> > distro/src/conf/atlas-application.properties
> > Lines 82 (patched)
> > <https://reviews.apache.org/r/60597/diff/1/?file=1768416#file1768416line82>
> >
> > Should this be 1000 ms? My thinking is 100 ms will be too short.
Yes 100 ms is small, i have changed it 1000 ms, some tuning might be required in that case this property atlas.kafka.poll.timeout.ms can be changed accordingly
- Nixon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60597/#review180395
-----------------------------------------------------------
On July 14, 2017, 9:52 a.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60597/
> -----------------------------------------------------------
>
> (Updated July 14, 2017, 9:52 a.m.)
>
>
> Review request for atlas, Apoorv Naik, Madhan Neethiraj, and Sarath Subramanian.
>
>
> Bugs: ATLAS-1908
> https://issues.apache.org/jira/browse/ATLAS-1908
>
>
> Repository: atlas
>
>
> Description
> -------
>
> This patch handles old kafka consumer configs values to be added as new configs if new configs are not present , so that Kafka consumer starts gracefully even if new configs are not added explicitly.
>
> eg. atlas.kafka.auto.commit.enable=false to atlas.kafka.enable.auto.commit=false
> atlas.kafka.auto.offset.reset=smallest to atlas.kafka.auto.offset.reset=earliest
> atlas.kafka.session.timeout.ms=30000
>
> Also added poll.timeout.ms in configs so kafka consumer can be configurable.This involve change in AtlasKafkaConsumer receive interface.
>
>
> Diffs
> -----
>
> distro/src/conf/atlas-application.properties c3213df
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java 9c15243
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 366c8a7
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java 22e40f9
> notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java 70059cb
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java c791d43
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 8324b57
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0dea0e2
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 650ca0a
> webapp/src/test/java/org/apache/atlas/web/integration/BaseResourceIT.java c036cfa
>
>
> Diff: https://reviews.apache.org/r/60597/diff/2/
>
>
> Testing
> -------
>
> Executed UT/IT's using mvn clean install.
> Tested KafkaConsumer with Hive Hook by creating table in hive.
> Atlas starts gracefully and Kafka consumer is up without error when new properties are not added.
>
>
> Thanks,
>
> Nixon Rodrigues
>
>
Re: Review Request 60597: ATLAS-1908 : - Updating old Kafka consumer
api
properties to reflect change in new KafkaConsumer configs for Atlas
Posted by Ashutosh Mestry <am...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60597/#review180395
-----------------------------------------------------------
distro/src/conf/atlas-application.properties
Lines 82 (patched)
<https://reviews.apache.org/r/60597/#comment255549>
Should this be 1000 ms? My thinking is 100 ms will be too short.
- Ashutosh Mestry
On July 3, 2017, 1:17 p.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60597/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 1:17 p.m.)
>
>
> Review request for atlas, Apoorv Naik, Madhan Neethiraj, and Sarath Subramanian.
>
>
> Bugs: ATLAS-1908
> https://issues.apache.org/jira/browse/ATLAS-1908
>
>
> Repository: atlas
>
>
> Description
> -------
>
> This patch handles old kafka consumer configs values to be added as new configs if new configs are not present , so that Kafka consumer starts gracefully even if new configs are not added explicitly.
>
> eg. atlas.kafka.auto.commit.enable=false to atlas.kafka.enable.auto.commit=false
> atlas.kafka.auto.offset.reset=smallest to atlas.kafka.auto.offset.reset=earliest
> atlas.kafka.session.timeout.ms=30000
>
> Also added poll.timeout.ms in configs so kafka consumer can be configurable.This involve change in AtlasKafkaConsumer receive interface.
>
>
> Diffs
> -----
>
> distro/src/conf/atlas-application.properties 474f253
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java 9c15243
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 366c8a7
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java 22e40f9
> notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java 70059cb
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java c791d43
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 8324b57
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 9e5b864
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 650ca0a
> webapp/src/test/java/org/apache/atlas/web/integration/BaseResourceIT.java c036cfa
>
>
> Diff: https://reviews.apache.org/r/60597/diff/1/
>
>
> Testing
> -------
>
> Executed UT/IT's using mvn clean install.
> Tested KafkaConsumer with Hive Hook by creating table in hive.
> Atlas starts gracefully and Kafka consumer is up without error when new properties are not added.
>
>
> Thanks,
>
> Nixon Rodrigues
>
>
Re: Review Request 60597: ATLAS-1908 : - Updating old Kafka consumer
api
properties to reflect change in new KafkaConsumer configs for Atlas
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
> On July 13, 2017, 5:23 p.m., Sarath Subramanian wrote:
> > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java
> > Line 126 (original), 127 (patched)
> > <https://reviews.apache.org/r/60597/diff/1/?file=1768418#file1768418line127>
> >
> > can the new kafka consumer handle both "smallest" and "earliest" value for auto.offset.reset?
the new kafka consumer can handle only earliest, smallest causes exception while initialization of kafkaconsumer. so set earliet as default which was set earlier.
> On July 13, 2017, 5:23 p.m., Sarath Subramanian wrote:
> > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java
> > Lines 133 (patched)
> > <https://reviews.apache.org/r/60597/diff/1/?file=1768418#file1768418line133>
> >
> > should this be subsetConfiguration.getLong("session.timeout.ms", 3000)?
Tried your suggestion but there was a parse error (below). The kafka config is expecting the long value in string format
272 org.apache.kafka.common.config.ConfigException: Invalid value 30000 for configuration session.timeout.ms: Expected value to be an number.
> On July 13, 2017, 5:23 p.m., Sarath Subramanian wrote:
> > notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java
> > Line 102 (original), 102 (patched)
> > <https://reviews.apache.org/r/60597/diff/1/?file=1768420#file1768420line102>
> >
> > why reduced poll timeout to 100L from 1000?
The return output of from poll method is mocked, so poll timeout will not matter as such, so set the timeout it less value to reduce execution time.
- Nixon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60597/#review180445
-----------------------------------------------------------
On July 14, 2017, 9:52 a.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60597/
> -----------------------------------------------------------
>
> (Updated July 14, 2017, 9:52 a.m.)
>
>
> Review request for atlas, Apoorv Naik, Madhan Neethiraj, and Sarath Subramanian.
>
>
> Bugs: ATLAS-1908
> https://issues.apache.org/jira/browse/ATLAS-1908
>
>
> Repository: atlas
>
>
> Description
> -------
>
> This patch handles old kafka consumer configs values to be added as new configs if new configs are not present , so that Kafka consumer starts gracefully even if new configs are not added explicitly.
>
> eg. atlas.kafka.auto.commit.enable=false to atlas.kafka.enable.auto.commit=false
> atlas.kafka.auto.offset.reset=smallest to atlas.kafka.auto.offset.reset=earliest
> atlas.kafka.session.timeout.ms=30000
>
> Also added poll.timeout.ms in configs so kafka consumer can be configurable.This involve change in AtlasKafkaConsumer receive interface.
>
>
> Diffs
> -----
>
> distro/src/conf/atlas-application.properties c3213df
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java 9c15243
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 366c8a7
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java 22e40f9
> notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java 70059cb
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java c791d43
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 8324b57
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0dea0e2
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 650ca0a
> webapp/src/test/java/org/apache/atlas/web/integration/BaseResourceIT.java c036cfa
>
>
> Diff: https://reviews.apache.org/r/60597/diff/2/
>
>
> Testing
> -------
>
> Executed UT/IT's using mvn clean install.
> Tested KafkaConsumer with Hive Hook by creating table in hive.
> Atlas starts gracefully and Kafka consumer is up without error when new properties are not added.
>
>
> Thanks,
>
> Nixon Rodrigues
>
>
Re: Review Request 60597: ATLAS-1908 : - Updating old Kafka consumer
api
properties to reflect change in new KafkaConsumer configs for Atlas
Posted by Sarath Subramanian <sa...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60597/#review180445
-----------------------------------------------------------
notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java
Line 126 (original), 127 (patched)
<https://reviews.apache.org/r/60597/#comment255628>
can the new kafka consumer handle both "smallest" and "earliest" value for auto.offset.reset?
notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java
Lines 133 (patched)
<https://reviews.apache.org/r/60597/#comment255629>
should this be subsetConfiguration.getLong("session.timeout.ms", 3000)?
notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java
Line 102 (original), 102 (patched)
<https://reviews.apache.org/r/60597/#comment255627>
why reduced poll timeout to 100L from 1000?
- Sarath Subramanian
On July 3, 2017, 6:17 a.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60597/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 6:17 a.m.)
>
>
> Review request for atlas, Apoorv Naik, Madhan Neethiraj, and Sarath Subramanian.
>
>
> Bugs: ATLAS-1908
> https://issues.apache.org/jira/browse/ATLAS-1908
>
>
> Repository: atlas
>
>
> Description
> -------
>
> This patch handles old kafka consumer configs values to be added as new configs if new configs are not present , so that Kafka consumer starts gracefully even if new configs are not added explicitly.
>
> eg. atlas.kafka.auto.commit.enable=false to atlas.kafka.enable.auto.commit=false
> atlas.kafka.auto.offset.reset=smallest to atlas.kafka.auto.offset.reset=earliest
> atlas.kafka.session.timeout.ms=30000
>
> Also added poll.timeout.ms in configs so kafka consumer can be configurable.This involve change in AtlasKafkaConsumer receive interface.
>
>
> Diffs
> -----
>
> distro/src/conf/atlas-application.properties 474f253
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java 9c15243
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 366c8a7
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java 22e40f9
> notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java 70059cb
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java c791d43
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 8324b57
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 9e5b864
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 650ca0a
> webapp/src/test/java/org/apache/atlas/web/integration/BaseResourceIT.java c036cfa
>
>
> Diff: https://reviews.apache.org/r/60597/diff/1/
>
>
> Testing
> -------
>
> Executed UT/IT's using mvn clean install.
> Tested KafkaConsumer with Hive Hook by creating table in hive.
> Atlas starts gracefully and Kafka consumer is up without error when new properties are not added.
>
>
> Thanks,
>
> Nixon Rodrigues
>
>
Re: Review Request 60597: ATLAS-1908 : - Updating old Kafka consumer
api
properties to reflect change in new KafkaConsumer configs for Atlas
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60597/#review180596
-----------------------------------------------------------
Ship it!
Ship It!
- Madhan Neethiraj
On July 14, 2017, 9:52 a.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60597/
> -----------------------------------------------------------
>
> (Updated July 14, 2017, 9:52 a.m.)
>
>
> Review request for atlas, Apoorv Naik, Madhan Neethiraj, and Sarath Subramanian.
>
>
> Bugs: ATLAS-1908
> https://issues.apache.org/jira/browse/ATLAS-1908
>
>
> Repository: atlas
>
>
> Description
> -------
>
> This patch handles old kafka consumer configs values to be added as new configs if new configs are not present , so that Kafka consumer starts gracefully even if new configs are not added explicitly.
>
> eg. atlas.kafka.auto.commit.enable=false to atlas.kafka.enable.auto.commit=false
> atlas.kafka.auto.offset.reset=smallest to atlas.kafka.auto.offset.reset=earliest
> atlas.kafka.session.timeout.ms=30000
>
> Also added poll.timeout.ms in configs so kafka consumer can be configurable.This involve change in AtlasKafkaConsumer receive interface.
>
>
> Diffs
> -----
>
> distro/src/conf/atlas-application.properties c3213df
> notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java 9c15243
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 366c8a7
> notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java 22e40f9
> notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java 70059cb
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java c791d43
> notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 8324b57
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0dea0e2
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 650ca0a
> webapp/src/test/java/org/apache/atlas/web/integration/BaseResourceIT.java c036cfa
>
>
> Diff: https://reviews.apache.org/r/60597/diff/2/
>
>
> Testing
> -------
>
> Executed UT/IT's using mvn clean install.
> Tested KafkaConsumer with Hive Hook by creating table in hive.
> Atlas starts gracefully and Kafka consumer is up without error when new properties are not added.
>
>
> Thanks,
>
> Nixon Rodrigues
>
>
Re: Review Request 60597: ATLAS-1908 : - Updating old Kafka consumer
api
properties to reflect change in new KafkaConsumer configs for Atlas
Posted by Nixon Rodrigues <ni...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60597/
-----------------------------------------------------------
(Updated July 14, 2017, 9:52 a.m.)
Review request for atlas, Apoorv Naik, Madhan Neethiraj, and Sarath Subramanian.
Changes
-------
Handled review comments from Sarath and Ashutosh.
Bugs: ATLAS-1908
https://issues.apache.org/jira/browse/ATLAS-1908
Repository: atlas
Description
-------
This patch handles old kafka consumer configs values to be added as new configs if new configs are not present , so that Kafka consumer starts gracefully even if new configs are not added explicitly.
eg. atlas.kafka.auto.commit.enable=false to atlas.kafka.enable.auto.commit=false
atlas.kafka.auto.offset.reset=smallest to atlas.kafka.auto.offset.reset=earliest
atlas.kafka.session.timeout.ms=30000
Also added poll.timeout.ms in configs so kafka consumer can be configurable.This involve change in AtlasKafkaConsumer receive interface.
Diffs (updated)
-----
distro/src/conf/atlas-application.properties c3213df
notification/src/main/java/org/apache/atlas/kafka/AtlasKafkaConsumer.java 9c15243
notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 366c8a7
notification/src/main/java/org/apache/atlas/notification/NotificationConsumer.java 22e40f9
notification/src/test/java/org/apache/atlas/kafka/KafkaConsumerTest.java 70059cb
notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java c791d43
notification/src/test/java/org/apache/atlas/notification/AbstractNotificationConsumerTest.java 8324b57
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 0dea0e2
webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerKafkaTest.java 650ca0a
webapp/src/test/java/org/apache/atlas/web/integration/BaseResourceIT.java c036cfa
Diff: https://reviews.apache.org/r/60597/diff/2/
Changes: https://reviews.apache.org/r/60597/diff/1-2/
Testing
-------
Executed UT/IT's using mvn clean install.
Tested KafkaConsumer with Hive Hook by creating table in hive.
Atlas starts gracefully and Kafka consumer is up without error when new properties are not added.
Thanks,
Nixon Rodrigues