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.

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

feat: allow balance strategies to provide initial state #1633

Merged
merged 3 commits into from
Sep 13, 2021

Conversation

aldelucca1
Copy link
Contributor

This update will allow stateful balance strategies to provide initial state information to send as part of the joinGroupRequest.

Previous to this PR if initial data was provided via c.config.Consumer.Group.Member.UserData it would always be sent when re-joining. This would trump the previous assignment data returned as part of the syncGroupRequest. After this PR the initial data will be sent when there is no currently assigned user-data that was returned as part of the assignment.

@aldelucca1 aldelucca1 requested a review from bai as a code owner February 28, 2020 14:08
@d1egoaz
Copy link
Contributor

d1egoaz commented Feb 28, 2020

Thanks @aldelucca1 could you please add first some tests for this? thanks

@aldelucca1
Copy link
Contributor Author

@d1egoaz been trying to get the Vagrant file on a fresh Vagrant install backed by VirtualBox to spin up so I can extend the functional test, but it fails to provision with the following error:

   default: + export KAFKA_INSTALL_ROOT=/opt
    default: + export KAFKA_HOSTNAME=192.168.100.67
    default: + export KAFKA_VERSION=1.0.2
    default: + export KAFKA_SCALA_VERSION=2.11
    default: + export REPOSITORY_ROOT=/vagrant
    default: + sh /vagrant/vagrant/install_cluster.sh
    default: + TOXIPROXY_VERSION=2.1.4
    default: + mkdir -p /opt
    default: + [ ! -f /opt/kafka-1.0.2.tgz ]
    default: + wget --quiet https://archive.apache.org/dist/kafka/1.0.2/kafka_2.11-1.0.2.tgz -O /opt/kafka-1.0.2.tgz
    default: + [ ! -f /opt/toxiproxy-2.1.4 ]
    default: + wget --quiet https://github.com/Shopify/toxiproxy/releases/download/v2.1.4/toxiproxy-server-linux-amd64 -O /opt/toxiproxy-2.1.4
    default: + chmod +x /opt/toxiproxy-2.1.4
    default: + rm -f /opt/toxiproxy
    default: + ln -s /opt/toxiproxy-2.1.4 /opt/toxiproxy
    default: + ZK_PORT=2181
    default: + ZK_PORT_REAL=21801
    default: + KAFKA_PORT=9091
    default: + KAFKA_PORT_REAL=29091
    default: + mkdir -p /opt/kafka-9091
    default: + tar xzf /opt/kafka-1.0.2.tgz -C /opt/kafka-9091 --strip-components 1
    default: + mkdir -p /opt/kafka-9091/data
    default: + printf \n\n
    default: + cat
    default: + cp /vagrant/vagrant/zookeeper.properties /opt/kafka-9091/config/
    default: + sed -i s/KAFKAID/9091/g /opt/kafka-9091/config/zookeeper.properties
    default: + sed -i s/ZK_PORT/21801/g /opt/kafka-9091/config/zookeeper.properties
    default: + ZK_DATADIR=/opt/zookeeper-2181
    default: + mkdir -p /opt/zookeeper-2181
    default: + sed -i s#ZK_DATADIR#/opt/zookeeper-2181#g /opt/kafka-9091/config/zookeeper.properties
    default: + echo 1
    default: + ZK_PORT=2182
    default: + ZK_PORT_REAL=21802
    default: + KAFKA_PORT=9092
    default: + KAFKA_PORT_REAL=29092
    default: + mkdir -p /opt/kafka-9092
    default: + tar xzf /opt/kafka-1.0.2.tgz -C /opt/kafka-9092 --strip-components 1
    default: + mkdir -p /opt/kafka-9092/data
    default: + printf \n\n
    default: + cat
    default: + cp /vagrant/vagrant/zookeeper.properties /opt/kafka-9092/config/
    default: + sed -i s/KAFKAID/9092/g /opt/kafka-9092/config/zookeeper.properties
    default: + sed -i s/ZK_PORT/21802/g /opt/kafka-9092/config/zookeeper.properties
    default: + ZK_DATADIR=/opt/zookeeper-2182
    default: + mkdir -p /opt/zookeeper-2182
    default: + sed -i s#ZK_DATADIR#/opt/zookeeper-2182#g /opt/kafka-9092/config/zookeeper.properties
    default: + echo 2
    default: + ZK_PORT=2183
    default: + ZK_PORT_REAL=21803
    default: + KAFKA_PORT=9093
    default: + KAFKA_PORT_REAL=29093
    default: + mkdir -p /opt/kafka-9093
    default: + tar xzf /opt/kafka-1.0.2.tgz -C /opt/kafka-9093 --strip-components 1
    default: + mkdir -p /opt/kafka-9093/data
    default: + printf \n\n
    default: + cat
    default: + cp /vagrant/vagrant/zookeeper.properties /opt/kafka-9093/config/
    default: + sed -i s/KAFKAID/9093/g /opt/kafka-9093/config/zookeeper.properties
    default: + sed -i s/ZK_PORT/21803/g /opt/kafka-9093/config/zookeeper.properties
    default: + ZK_DATADIR=/opt/zookeeper-2183
    default: + mkdir -p /opt/zookeeper-2183
    default: + sed -i s#ZK_DATADIR#/opt/zookeeper-2183#g /opt/kafka-9093/config/zookeeper.properties
    default: + echo 3
    default: + ZK_PORT=2184
    default: + ZK_PORT_REAL=21804
    default: + KAFKA_PORT=9094
    default: + KAFKA_PORT_REAL=29094
    default: + mkdir -p /opt/kafka-9094
    default: + tar xzf /opt/kafka-1.0.2.tgz -C /opt/kafka-9094 --strip-components 1
    default: + mkdir -p /opt/kafka-9094/data
    default: + printf \n\n
    default: + cat
    default: + cp /vagrant/vagrant/zookeeper.properties /opt/kafka-9094/config/
    default: + sed -i s/KAFKAID/9094/g /opt/kafka-9094/config/zookeeper.properties
    default: + sed -i s/ZK_PORT/21804/g /opt/kafka-9094/config/zookeeper.properties
    default: + ZK_DATADIR=/opt/zookeeper-2184
    default: + mkdir -p /opt/zookeeper-2184
    default: + sed -i s#ZK_DATADIR#/opt/zookeeper-2184#g /opt/kafka-9094/config/zookeeper.properties
    default: + echo 4
    default: + ZK_PORT=2185
    default: + ZK_PORT_REAL=21805
    default: + KAFKA_PORT=9095
    default: + KAFKA_PORT_REAL=29095
    default: + mkdir -p /opt/kafka-9095
    default: + tar xzf /opt/kafka-1.0.2.tgz -C /opt/kafka-9095 --strip-components 1
    default: + mkdir -p /opt/kafka-9095/data
    default: + printf \n\n
    default: + cat
    default: + cp /vagrant/vagrant/zookeeper.properties /opt/kafka-9095/config/
    default: + sed -i s/KAFKAID/9095/g /opt/kafka-9095/config/zookeeper.properties
    default: + sed -i s/ZK_PORT/21805/g /opt/kafka-9095/config/zookeeper.properties
    default: + ZK_DATADIR=/opt/zookeeper-2185
    default: + mkdir -p /opt/zookeeper-2185
    default: + sed -i s#ZK_DATADIR#/opt/zookeeper-2185#g /opt/kafka-9095/config/zookeeper.properties
    default: + echo 5
    default: + sh /vagrant/vagrant/setup_services.sh
    default: + 
    default: stop
    default:  toxiproxy
    default: /vagrant/vagrant/setup_services.sh: 5: /vagrant/vagrant/setup_services.sh: 
    default: stop: not found
    default: + 
    default: true
    default: + 
    default: cp
    default:  /vagrant/vagrant/toxiproxy.conf
    default:  /etc/init/toxiproxy.conf
    default: cp: cannot create regular file '/etc/init/toxiproxy.conf': No such file or directory

I've tried older versions of ubuntu/bionic64 thinking maybe there was a breaking change in the newer builds but didn't have much success. Any pointers?

@aldelucca1
Copy link
Contributor Author

@bai would you be able to provide some pointers here?

@aldelucca1
Copy link
Contributor Author

Turns out the Vagrant file is referencing a version of ubuntu that doesn't have Upstart. Will fix it as part of this PR

@aldelucca1
Copy link
Contributor Author

@d1egoaz this should be ready for a look

@aldelucca1
Copy link
Contributor Author

aldelucca1 commented Apr 30, 2020

The test seem flaky after pulling in the latest master, seems to be failing intermittently on different checks attempting to install the dependencies

@aldelucca1
Copy link
Contributor Author

The test seem flaky after pulling in the latest master, seems to be failing intermittently on different checks attempting to install the dependencies

Seems good to go now

@ghost ghost added the stale Issues and pull requests without any recent activity label Mar 16, 2021
@dnwe
Copy link
Collaborator

dnwe commented Mar 18, 2021

@aldelucca1 sorry for the late reply on this PR, these changes look reasonable to me — please can you rebase on master and then I can re-review and merge?

@ghost ghost removed the stale Issues and pull requests without any recent activity label Mar 18, 2021
@IBM IBM deleted a comment Mar 18, 2021
aldelucca1 and others added 3 commits September 13, 2021 09:40
This update will allow stateful balance strategies to provide initial state information to send as part of the `joinGroupRequest`.

Previous to this PR if initial data was provided via `c.config.Consumer.Group.Member.UserData` it would always be sent when re-joining. This would trump the previous assignment data returned as part of the `syncGroupRequest`. After this PR the initial data will be sent when there is no currently assigned user-data that was returned as part of the assignment.
@bai bai force-pushed the allow-initial-group-state branch from d730393 to bd3257f Compare September 13, 2021 07:01
@bai bai requested a review from dnwe September 13, 2021 07:27
@bai
Copy link
Contributor

bai commented Sep 13, 2021

@dnwe requesting re-review since I rebased this PR off of master.

Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for that

@dnwe dnwe changed the title Allow balance strategies to provide initial state feat: allow balance strategies to provide initial state Sep 13, 2021
@dnwe dnwe merged commit b83b13f into IBM:main Sep 13, 2021
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.

4 participants