ConsumeKafka_2_0 Max Poll Records

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

ConsumeKafka_2_0 Max Poll Records

Jason Iannone
Hi all,

I have been digging through the ConsumeKafka_2_0 (and record) code and noticed that the PropertyDescriptor for MAX_POLL_RECORDS isn't connected to anything. Is this intentional, and a "deprecated field" or am I missing something?

Thanks,
Jason
Reply | Threaded
Open this post in threaded view
|

Re: ConsumeKafka_2_0 Max Poll Records

Bryan Bende
Hi Jason,

It is a bit confusing, but in the createConsumerPool there is a line:

KafkaProcessorUtils.buildCommonKafkaProperties(context, ConsumerConfig.class, props);

That ends up looping through all the properties of the processor and checking if their name matches any of the names in ConsumerConfig, and if so it adds the key/value to the config map.

So since the property is named "max.poll.records", that is the same name in ConsumerConfig.

Thanks,

Bryan

On Tue, Jul 7, 2020 at 9:32 AM Jason Iannone <[hidden email]> wrote:
Hi all,

I have been digging through the ConsumeKafka_2_0 (and record) code and noticed that the PropertyDescriptor for MAX_POLL_RECORDS isn't connected to anything. Is this intentional, and a "deprecated field" or am I missing something?

Thanks,
Jason
Reply | Threaded
Open this post in threaded view
|

Re: ConsumeKafka_2_0 Max Poll Records

Jason Iannone
Hi Bryan,

Thanks, I completely missed that! It also makes more sense why some of the properties use dot notation over dash. Definitely something that there should be tests around to make sure it isn't accidentally removed.

Thanks,
Jason

On Tue, Jul 7, 2020 at 9:48 AM Bryan Bende <[hidden email]> wrote:
Hi Jason,

It is a bit confusing, but in the createConsumerPool there is a line:

KafkaProcessorUtils.buildCommonKafkaProperties(context, ConsumerConfig.class, props);

That ends up looping through all the properties of the processor and checking if their name matches any of the names in ConsumerConfig, and if so it adds the key/value to the config map.

So since the property is named "max.poll.records", that is the same name in ConsumerConfig.

Thanks,

Bryan

On Tue, Jul 7, 2020 at 9:32 AM Jason Iannone <[hidden email]> wrote:
Hi all,

I have been digging through the ConsumeKafka_2_0 (and record) code and noticed that the PropertyDescriptor for MAX_POLL_RECORDS isn't connected to anything. Is this intentional, and a "deprecated field" or am I missing something?

Thanks,
Jason
Reply | Threaded
Open this post in threaded view
|

Re: ConsumeKafka_2_0 Max Poll Records

Mark Payne
Adding unit tests is not a bad idea. But really I think we should just refactor this and properly use the properties explicitly. The current implementation is a bit “magical” and not at all straight forward.


On Jul 7, 2020, at 9:58 AM, Jason Iannone <[hidden email]> wrote:

Hi Bryan,

Thanks, I completely missed that! It also makes more sense why some of the properties use dot notation over dash. Definitely something that there should be tests around to make sure it isn't accidentally removed.

Thanks,
Jason

On Tue, Jul 7, 2020 at 9:48 AM Bryan Bende <[hidden email]> wrote:
Hi Jason,

It is a bit confusing, but in the createConsumerPool there is a line:

KafkaProcessorUtils.buildCommonKafkaProperties(context, ConsumerConfig.class, props);

That ends up looping through all the properties of the processor and checking if their name matches any of the names in ConsumerConfig, and if so it adds the key/value to the config map.

So since the property is named "max.poll.records", that is the same name in ConsumerConfig.

Thanks,

Bryan

On Tue, Jul 7, 2020 at 9:32 AM Jason Iannone <[hidden email]> wrote:
Hi all,

I have been digging through the ConsumeKafka_2_0 (and record) code and noticed that the PropertyDescriptor for MAX_POLL_RECORDS isn't connected to anything. Is this intentional, and a "deprecated field" or am I missing something?

Thanks,
Jason

Reply | Threaded
Open this post in threaded view
|

Re: ConsumeKafka_2_0 Max Poll Records

Jason Iannone
Mark, agreed. I like to refactor by making sure I have tests, that way I know what I broke.  I'm hoping this is an area I can help with once I get through the red tape.

On Tue, Jul 7, 2020 at 10:14 AM Mark Payne <[hidden email]> wrote:
Adding unit tests is not a bad idea. But really I think we should just refactor this and properly use the properties explicitly. The current implementation is a bit “magical” and not at all straight forward.


On Jul 7, 2020, at 9:58 AM, Jason Iannone <[hidden email]> wrote:

Hi Bryan,

Thanks, I completely missed that! It also makes more sense why some of the properties use dot notation over dash. Definitely something that there should be tests around to make sure it isn't accidentally removed.

Thanks,
Jason

On Tue, Jul 7, 2020 at 9:48 AM Bryan Bende <[hidden email]> wrote:
Hi Jason,

It is a bit confusing, but in the createConsumerPool there is a line:

KafkaProcessorUtils.buildCommonKafkaProperties(context, ConsumerConfig.class, props);

That ends up looping through all the properties of the processor and checking if their name matches any of the names in ConsumerConfig, and if so it adds the key/value to the config map.

So since the property is named "max.poll.records", that is the same name in ConsumerConfig.

Thanks,

Bryan

On Tue, Jul 7, 2020 at 9:32 AM Jason Iannone <[hidden email]> wrote:
Hi all,

I have been digging through the ConsumeKafka_2_0 (and record) code and noticed that the PropertyDescriptor for MAX_POLL_RECORDS isn't connected to anything. Is this intentional, and a "deprecated field" or am I missing something?

Thanks,
Jason