Skip to content

KAFKA-8599: Use automatic RPC generation in ExpireDelegationToken#7098

Closed
mimaison wants to merge 1 commit into
apache:trunkfrom
mimaison:KAFKA-8599
Closed

KAFKA-8599: Use automatic RPC generation in ExpireDelegationToken#7098
mimaison wants to merge 1 commit into
apache:trunkfrom
mimaison:KAFKA-8599

Conversation

@mimaison

Copy link
Copy Markdown
Member

Committer Checklist (excluded from commit message)

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

@mimaison

Copy link
Copy Markdown
Member Author

@abbccdda @cmccabe Can you take a look?

@viktorsomogyi viktorsomogyi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

ByteBuffer buf = (ByteBuffer) result;
byte[] arr = new byte[buf.remaining()];
buf.get(arr);
buf.flip();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice find.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On the other hand I was debating whether it's worth to use buf.duplicate().get() instead of flipping on the expense of creating a lightweight ByteBuffer object. Advantages is that we don't mutate the current buffer but instead work on a throwaway object.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I too was unsure which way to go. I'm happy with either solutions, let's see what a committer thinks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mimaison Is this causing any issue? If not, Can we just add javadoc saying "Reads the remaining bytes in a ByteBuffer into a byte[]." and retain current code.

cc @ijuma

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

By not flipping the buffer, the position stays at the end. Then when comparing Requests, the ones built via a Struct were not equals to the ones built directly.

@mimaison

mimaison commented Aug 5, 2019

Copy link
Copy Markdown
Member Author

@omkreddy Can you have a look?

@omkreddy omkreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mimaison Thanks for the PR. LGTM.

@omkreddy omkreddy closed this in 926fb35 Aug 7, 2019
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.

3 participants