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

added offline replicas operation to client for kafka version >= 1.x #1318

Merged
merged 4 commits into from
Apr 3, 2019

Conversation

drsoares
Copy link
Contributor

@drsoares drsoares commented Mar 17, 2019

Added Offline Replicas Operation to Client.

  • Added OfflineReplicas Operation
  • Implemented Operation
  • Added unit tests
  • Changed the metadata request and response encoders
  • Add to add logic to set the version id based on the version present on the Config

#1311

@@ -42,7 +42,7 @@ func (r *MetadataRequest) decode(pd packetDecoder, version int16) error {
} else {
topicCount := size
if topicCount == 0 {
return nil
goto SKIP
Copy link
Contributor

Choose a reason for hiding this comment

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

why not return here?

Copy link
Contributor Author

@drsoares drsoares Apr 1, 2019

Choose a reason for hiding this comment

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

If the protocol version is higher than 3 (0.11.0.0), which is the case of version 1.0.0.0 (5) that supports the offline replicas request, the if block on the bottom of the function would be skipped:

if r.Version > 3 {
   autoCreation, err := pd.getBool()
   if err != nil {
      return err
   }
   r.AllowAutoTopicCreation = autoCreation
}

But obviously I admit this is a potential point of improvement, I can try to refactor this to a more idiomatic way.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, please have a look, if it's can't be refactored, I am not super against GOTO's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@bai bai requested a review from varun06 April 2, 2019 13:10
@bai bai merged commit a4a1b88 into IBM:master Apr 3, 2019
@gbbr
Copy link

gbbr commented Apr 10, 2019

This breaks the API, so you should probably bump your next major version to 2.x since this technically isn't 1.x anymore.

gbbr added a commit to DataDog/dd-trace-go that referenced this pull request Apr 10, 2019
gbbr added a commit to DataDog/dd-trace-go that referenced this pull request Apr 10, 2019
gbbr added a commit to DataDog/dd-trace-go that referenced this pull request Apr 10, 2019
gbbr added a commit to DataDog/dd-trace-go that referenced this pull request Apr 10, 2019
gbbr added a commit to DataDog/dd-trace-go that referenced this pull request Apr 10, 2019
@varun06
Copy link
Contributor

varun06 commented Apr 10, 2019

Apologies for missing it @gbbr
I also had to fix an interface after upgrading to this. I am gonna work with @bai and team to handle breaking changes better in future.

@drsoares
Copy link
Contributor Author

Can you please clarify where were the breaking changes introduced? Apologies in advance.

@varun06
Copy link
Contributor

varun06 commented Apr 10, 2019

OfflineReplicas was a new method in client interface. We have some internal stuff where we implement the interface. So when I ran tests with new version, it was complaining for that method, I implemented it and all fine. But it seems that @gbbr had a breaking build too.

@drsoares
Copy link
Contributor Author

There was however a breaking change introduced on the testing api, however there's nothing stoping you from using those methods on your code as helpers, perhaps they should be moved into some internal package.

gbbr added a commit to DataDog/dd-trace-go that referenced this pull request Apr 10, 2019
@gbbr
Copy link

gbbr commented Apr 15, 2019

You've actually motivated me to write this (very experimental) package over the weekend: https://github.com/gbbr/breakcheck

@varun06
Copy link
Contributor

varun06 commented Apr 15, 2019

@gbbr that's great, I will have a look and might help with some issues(time permitted).

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