Skip to content

Kafka-6693: Added consumer workload to Trogdor#4775

Merged
rajinisivaram merged 5 commits intoapache:trunkfrom
apovzner:consumer-bench
Apr 10, 2018
Merged

Kafka-6693: Added consumer workload to Trogdor#4775
rajinisivaram merged 5 commits intoapache:trunkfrom
apovzner:consumer-bench

Conversation

@apovzner
Copy link
Copy Markdown
Contributor

Added consumer only workload to Trogdor. The topics must already be pre-populated. The spec lets the user request topic pattern and range of partitions to assign to [startPartition, endPartition].

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be CREATE_TOPICS_REQUEST_TIMEOUT for consistency

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a clientConf as well, right-- as per the discussion earlier about admin client security configs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, added commonConf and adminClientConf.

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Mar 27, 2018

Looks good overall. Thanks, @apovzner

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apovzner Thanks for the PR. Looks good, left a few minor comments.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should change the name of CREATE_TOPICS_REQUEST_TIMEOUT since it used as timeout for ListTopicsRequest and DescribeTopicsRequest.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it is better to check isInternal first to avoid doing the regex match unnecessarily?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels odd to set this.consumer to null here. I was expecting any previous value to be set to null immediately after the consumer is closed. Otherwise, there ought to be a consumer.close() before setting to null. Since stop closes and sets consumer to null, perhaps we dont need this here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, stop() closes the consumer and sets to null anyway, and we guard agains multiple start() or close() calls. I will remove setting it to null here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove blank line?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use either records or messages consistently?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change to ConsumeMessages since we use word message everywhere.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use Utils.mkSet()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use Utils.mkSet

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apovzner Thanks for the updates, LGTM

@rajinisivaram
Copy link
Copy Markdown
Contributor

Merging to trunk.

@rajinisivaram rajinisivaram merged commit 989fe04 into apache:trunk Apr 10, 2018
@apovzner apovzner deleted the consumer-bench branch April 10, 2018 16:07
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
Added consumer only workload to Trogdor. The topics must already be pre-populated. The spec lets the user request topic pattern and range of partitions to assign to [startPartition, endPartition].

Reviewers: Colin P. Mccabe <cmccabe@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants