Skip to content

Comments

MINOR: follow-up test for create topic acl (KIP-277)#5155

Merged
ijuma merged 3 commits intoapache:trunkfrom
edoardocomar:topic_create_acl_followup_test
Jun 13, 2018
Merged

MINOR: follow-up test for create topic acl (KIP-277)#5155
ijuma merged 3 commits intoapache:trunkfrom
edoardocomar:topic_create_acl_followup_test

Conversation

@edoardocomar
Copy link
Contributor

Check handling of a Metadata Request with a mix of existing and
non-existing topics.

The particular codepath was not exercised by a test in the original PR.
@ijuma fixed the code path on merge
This small testcase covers that

Co-authored-by: Edoardo Comar ecomar@uk.ibm.com
Co-authored-by: Mickael Maison mickael.maison@gmail.com

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

@edoardocomar edoardocomar force-pushed the topic_create_acl_followup_test branch 2 times, most recently from 537b9f8 to b02a04c Compare June 8, 2018 09:52
edoardocomar and others added 3 commits June 11, 2018 00:29
Check handling of a Metadata request with a mix of existing and
non-existing topics.

Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com>
Co-authored-by: Mickael Maison <mickael.maison@gmail.com>
@edoardocomar edoardocomar force-pushed the topic_create_acl_followup_test branch from b02a04c to 96b1228 Compare June 10, 2018 23:41
@edoardocomar
Copy link
Contributor Author

@ijuma I have rebased this PR a few times. would you like to merge it ? thanks

Copy link
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM. I assume this test fails if we make the change that we had in the previous PR?

@ijuma ijuma merged commit 477ae9e into apache:trunk Jun 13, 2018
@edoardocomar
Copy link
Contributor Author

thanks @ijuma - yes this test failed without this fix dc2727e of yours in the other PR

@edoardocomar edoardocomar deleted the topic_create_acl_followup_test branch June 13, 2018 09:11
@edoardocomar
Copy link
Contributor Author

Hi @ijuma I thought you were going to commit to the 2.0 branch as well ?

@ijuma
Copy link
Member

ijuma commented Jun 13, 2018

I only merged to trunk. PRs target trunk so the benefit of backporting seems minimal.

@edoardocomar
Copy link
Contributor Author

I am not sure I understand ... KIP-277 is in 2.0, #4795 was committed to trunk before the 2.0 branch was created.
So I think the test gap could be closed there as well

@edoardocomar
Copy link
Contributor Author

@ijuma ^

@ijuma
Copy link
Member

ijuma commented Jun 15, 2018

The testing gap existed previously though. By your logic, every time we find a testing gap, we need to backport the fix to every branch. In any case, since you seem to care a lot about this (even if the reason is unclear to me), I will cherry-pick it to 2.0 (it's harmless, but taking committer time on something with minimal benefit compared to other things).

ijuma pushed a commit that referenced this pull request Jun 15, 2018
…isting topics (#5155)

A bug in the original KIP-277 submission was caught during code review,
but it was not detected by the tests. Fix that gap.

Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com>
Co-authored-by: Mickael Maison <mickael.maison@gmail.com>
@edoardocomar
Copy link
Contributor Author

I understand that committer time is a scarce resource, but I thought the testing gap was due to the introduction of KIP-277 in 2.0.

ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
…isting topics (apache#5155)

A bug in the original KIP-277 submission was caught during code review,
but it was not detected by the tests. Fix that gap.

Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com>
Co-authored-by: Mickael Maison <mickael.maison@gmail.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.

2 participants