fix-#181: Send the ack token as String #182
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
komamitsu
reviewed
Feb 6, 2021
| @@ -71,12 +66,15 @@ public void ingest(String tag, ByteBuffer dataBuffer) | |||
| ByteBuffer headerBuffer = ByteBuffer.wrap(header.toByteArray()); | |||
|
|
|||
| if (config.isAckResponseMode()) { | |||
| byte[] uuidBytes = UUID.randomUUID().toString().getBytes(CHARSET); | |||
| ByteBuffer optionBuffer = ByteBuffer.wrap(objectMapper.writeValueAsBytes(new RequestOption(dataLength, uuidBytes))); | |||
| // The spec says to encode a 128 bit value as base64, but fluent-bit currently | |||
There was a problem hiding this comment.
Which document does it mean by The spec?
| ByteBuffer optionBuffer = ByteBuffer.wrap(objectMapper.writeValueAsBytes(new RequestOption(dataLength, uuidBytes))); | ||
| // The spec says to encode a 128 bit value as base64, but fluent-bit currently | ||
| // uses a 32 char long hex string, so we do the same for now. | ||
| String token = UUID.randomUUID().toString().replace("-", ""); |
There was a problem hiding this comment.
ACK token formats in Fluentd and Fluent Bit are different and there is no validation of it. How about simply passing a UUID hex string as is without removing hyphen? If Fluentd and Fluent Bit implement a validation of ACK token in the future, it would be time to change this format in Fluency as well.
There was a problem hiding this comment.
Fixed. I still think it is better to comply with the spec straight away, but I am also fine with going ahead like this for now.
There was a problem hiding this comment.
Thanks!
Let's follow the spec later after Fluentd and Fluent bit use a consistent format.
komamitsu
approved these changes
Feb 11, 2021
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi!
As discussed in the ticket, here the fix. I fixed small issues I found around the places I changed, too.
Cheers
Ruwen
PS: I struggled a bit with the code style, I hope I didn't miss anything.