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

Ack handler #2516

Merged
merged 29 commits into from
Jul 31, 2019
Merged

Ack handler #2516

merged 29 commits into from
Jul 31, 2019

Conversation

ganmacs
Copy link
Member

@ganmacs ganmacs commented Jul 22, 2019

Which issue(s) this PR fixes:
no

What this PR does / why we need it:

Extract the code around ack_response as AckHandler class.
It aims to simplify out_forward and add more test about it.

Docs Changes:

no needed

Release Note:

no needed

@ganmacs ganmacs self-assigned this Jul 24, 2019
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Fluent::Engine.msgpack_unpacker
Fluent::PluginHelper::Socket::WrappedSocket::TLS

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs ganmacs marked this pull request as ready for review July 26, 2019 02:38
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs ganmacs requested a review from repeatedly July 26, 2019 02:40
@ganmacs
Copy link
Member Author

ganmacs commented Jul 30, 2019

@repeatedly ping?

rollback_write(chunk_id, update_retry: false)
when AckHandler::Result::CHUNKID_UNMATCHED
rollback_write(chunk_id, update_retry: false)
else
Copy link
Member

Choose a reason for hiding this comment

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

result is 3 types so this else flow is for catching implementation bug, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. this condition is for the case I don't expect now happen.

Copy link
Member

Choose a reason for hiding this comment

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

If so, add BUG: to log message is better for notification.

log.warn "BUG: failed to verify certification while connecting (but not raised, why?)", host: host, fqdn: fqdn, error: err_name

end
end

ACKWaitingSockInfo = Struct.new(:sock, :chunk_id, :chunk_id_base64, :node, :time, :timeout) do
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember why this struct has time and timeout fields and calculate expired time in expired? call.
Could we change this struct like below?

ACKWaitingSockInfo = Struct.new(:sock, :chunk_id, :chunk_id_base64, :node, :expired_time) do
  def expired?(now)
    expired_time < now
  end
end

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
Copy link
Member Author

ganmacs commented Jul 31, 2019

Thank you for the review!

@ganmacs ganmacs merged commit 9b17037 into fluent:master Jul 31, 2019
@ganmacs ganmacs deleted the ack_handler branch July 31, 2019 03:34
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.

2 participants