Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Topic Operator startup #1542

Merged
merged 7 commits into from
Apr 16, 2019
Merged

Topic Operator startup #1542

merged 7 commits into from
Apr 16, 2019

Conversation

tombentley
Copy link
Member

Type of change

  • Bugfix

Description

This PR fixes a number of Topic Operator bugs and problems in the tests:

  1. The startup behaviour of the TO wasn't properly tested. A test is added to the TopicOperatorIntegrationTest.
  2. This test found a situation where the TO would continuously reconcile topic(s), this bug is fixed.
  3. Another TO in the reconciliation was fixed by @eye0fra.
  4. A bug in Session shutdown. This was unlikely to be problematic in practice since the TO typically just gets killed, but was a problem for test reliability.
  5. Various races in the TOIT are fixed (e.g. the test now uses its own AdminClient instance rather than using the one from Session).

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Update/write design documentation in ./design
  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md

eye0fra and others added 7 commits April 15, 2019 10:59
@eye0fra @tombentley
Update TopicOperator.java
a859e69 
Fix reconcileFromKafka to correctly save the state of the topic
@eye0fra @tombentley
Update TopicOperator.java
8dab052
@tombentley
* Continual reconciliation on startup
3cb79eb 
* Factor out getKafkaAndReconcile()
* Avoid CountDownLatch, use futures, fix ReconciledState.failed
* Make AdminClient private in Session
@tombentley
* Add new test for behaviour on TO startup
b00ff6e 
* Test refactoring
* Describe can throw InvalidTopicException in TOIT
* Wait for topic deletion when cleaning up after the test
@tombentley
Fix loggin in inflights
357ace0
@tombentley
Fix timeout in Session
0903eb1
@tombentley
Fix double-completed handler in TopicOperator
d05ba90
@scholzj scholzj requested a review from sknot-rh April 15, 2019 15:09
scholzj
scholzj approved these changes Apr 15, 2019
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

LGTM. But I requested a review from @stanlyDoge who AFAIK know more about TO than I do.

sknot-rh
sknot-rh approved these changes Apr 15, 2019
View reviewed changes
Copy link
Member

@sknot-rh sknot-rh left a comment

Choose a reason for hiding this comment

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

LGTM

@tombentley tombentley merged commit 9f42a00 into master Apr 16, 2019
@tombentley tombentley deleted the to-startup branch April 16, 2019 17:01
@tombentley tombentley added this to the 0.12.0 milestone Apr 16, 2019
@tombentley tombentley mentioned this pull request Apr 16, 2019
Closed
8 tasks
tombentley added a commit that referenced this pull request Apr 29, 2019
@tombentley
Topic Operator startup (#1542)
ee97d13 
* Fix reconcileFromKafka to correctly save the state of the topic
* Continual reconciliation on startup
* Factor out getKafkaAndReconcile()
* Avoid CountDownLatch, use futures, fix ReconciledState.failed
* Make AdminClient private in Session
* Fix double-completed handler in TopicOperator
* Add new test for behaviour on TO startup
* Describe can throw InvalidTopicException in TOIT
* Wait for topic deletion when cleaning up after the test
* Fix logging in InFlight
* Fix timeout in Session
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scholzj scholzj scholzj approved these changes

@sknot-rh sknot-rh sknot-rh approved these changes

Assignees

@scholzj scholzj

Labels
None yet
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tombentley @scholzj @sknot-rh @eye0fra