Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[incubator/kafka]: add reassignPartitions to topic configuration #7623

Merged
merged 2 commits into from
Sep 19, 2018
Merged

[incubator/kafka]: add reassignPartitions to topic configuration #7623

merged 2 commits into from
Sep 19, 2018

Conversation

giacomoguiulfo
Copy link
Collaborator

@giacomoguiulfo giacomoguiulfo commented Sep 9, 2018 •
edited
Loading

What this PR does / why we need it:

The only way to increase the replication factor of an existing topic, is by reassigning partitions. This PR adds a new field to the topic configuration values: reassignPartitions. When this value is set to true, the kafka-config-job will reassign partitions and increase the replication factor of an existing topic. Both partitions and replicationFactor need to be specified as well.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 9, 2018
@giacomoguiulfo
Copy link
Collaborator Author

/assign @davidkarlsen

@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 9, 2018
@benjigoldberg
Copy link
Collaborator

/assign @benjigoldberg
/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 10, 2018
@davidkarlsen
Copy link
Member

@benjigoldberg is this ok to merge?

@benjigoldberg
Copy link
Collaborator

@davidkarlsen yes
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 19, 2018
@giacomoguiulfo
[incubator/kafka]: add reassignPartitions to topic configuration
8375e2d 
Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>
@giacomoguiulfo
[incubator/kafka]: bump chart to 0.10.0
ac27261 
Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2018
@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 19, 2018
@giacomoguiulfo
Copy link
Collaborator Author

@benjigoldberg (cc @davidkarlsen)
The PR needed a bump in the chart version in order to pass CI. So, I just did that and also decided to do a minor version bump (from 0.9.6 to 0.10.0) since this is a feature rather than a patch. But let me know if you guys prefer just a "patch" to 0.9.7.

@benjigoldberg
Copy link
Collaborator

Thanks @giacomoguiulfo -- apologies for the delay on merging this.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjigoldberg, giacomoguiulfo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit ceb14f1 into helm:master Sep 19, 2018
@giacomoguiulfo giacomoguiulfo deleted the feature/kafka-reassign-partitions branch September 19, 2018 22:10
legal90 added a commit to volvo-cars/helm-charts that referenced this pull request Sep 21, 2018
@legal90
Merge branch 'superset-annotations' into volvo-custom
da753e7 
* superset-annotations: (27 commits)
  [stable/superset] Bump version to 0.1.3
  [stable/superset] Add service annotations support
  [stable/drupal] Fix chart not being upgradable (helm#7825)
  [stable/parse] Fix chart not being upgradable (helm#7824)
  [stable/jasperreports] Fix chart not being upgradable (helm#7818)
  [stable/osclass] Fix chart not being upgradable (helm#7817)
  [stable/phpbb] Fix chart not being upgradable (helm#7820)
  [stable/ghost] Fix chart not being upgradable (helm#7814)
  [stable/suitecrm] Fix chart not being upgradable (helm#7816)
  [stable/phpmyadmin] Fix chart not being upgradable (helm#7830)
  [stable/magento] Release 2.0.6 (helm#7810)
  [stable/wordpress] Fix chart not being upgradable (helm#7831)
  [stable/mongodb] Use .Values.existingSecret for standalone deployments (helm#7839)
  [incubator/kafka]: add reassignPartitions to topic configuration (helm#7623)
  Changed syntax error in custom-metrics-apiserver-service and secret (label to labels) (helm#7295)
  [stable/spinnaker] Changing email of dwardu89 (helm#7838)
  fix: mongo init issues (helm#7772)
  [stable/mattemost-team-edition] Add initial charts for Mattermost Team Edition (helm#5987)
  Fixed TLS Ingress, updated mongo dep requirements and app to latest version (helm#7636)
  Adding dwardu89 to owners and chart maintainers of stable/spinnaker (helm#7751)
  ...
jicowan pushed a commit to jicowan/charts that referenced this pull request Oct 2, 2018
@giacomoguiulfo @jicowan
[incubator/kafka]: add reassignPartitions to topic configuration (hel…
75fa411 
…m#7623)

* [incubator/kafka]: add reassignPartitions to topic configuration

Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>

* [incubator/kafka]: bump chart to 0.10.0

Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>
Signed-off-by: jenkin-x <jicowan@hotmail.com>
@vanto
Copy link
Contributor

vanto commented Oct 4, 2018

Wouldn't it be better to move the if statement from the template to runtime? Currently, if the reassignPartitions flag is set, non-existing topics are not created anymore. What about changing the script to the following behaviour:

  1. check if topic exists. If no, create and return. If yes:
  2. check if reassignPartitions flag is set. If no, return, if yes, do the assignment.

WDYT?

@giacomoguiulfo
Copy link
Collaborator Author

@vanto If it works, it sounds good to me. I agree this is not the best way to do things.

Jnig pushed a commit to Jnig/charts that referenced this pull request Nov 13, 2018
@giacomoguiulfo @Jnig
[incubator/kafka]: add reassignPartitions to topic configuration (hel…
c813cdf 
…m#7623)

* [incubator/kafka]: add reassignPartitions to topic configuration

Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>

* [incubator/kafka]: bump chart to 0.10.0

Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>
Signed-off-by: Jakob Niggel <info@jakobniggel.de>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
@giacomoguiulfo @wgiddens
[incubator/kafka]: add reassignPartitions to topic configuration (hel…
3e559ae 
…m#7623)

* [incubator/kafka]: add reassignPartitions to topic configuration

Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>

* [incubator/kafka]: bump chart to 0.10.0

Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benjigoldberg benjigoldberg Awaiting requested review from benjigoldberg

@viglesiasce viglesiasce Awaiting requested review from viglesiasce

Assignees

@davidkarlsen davidkarlsen

@benjigoldberg benjigoldberg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@giacomoguiulfo @benjigoldberg @davidkarlsen @k8s-ci-robot @vanto @ey-bot