Skip to content

Set timeout on splunk batch send #35496

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

Merged
merged 1 commit into from
Nov 30, 2017

Conversation

cpuguy83
Copy link
Member

Set timeout on splunk batch send

Before this change, if the splunk endpoint is blocked it will cause a
deadlock on Close().
This sets a reasonable timeout for the http request to send a log batch.

Before this change, if the splunk endpoint is blocked it will cause a
deadlock on `Close()`.
This sets a reasonable timeout for the http request to send a log batch.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@@ -63,6 +64,8 @@ const (
envVarStreamChannelSize = "SPLUNK_LOGGING_DRIVER_CHANNEL_SIZE"
)

var batchSendTimeout = 30 * time.Second
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 const?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm changing it to a shorter timeout for tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@yongtang
Copy link
Member

Ping @anusha-ragunathan please take a look and see if it is OK.

@anusha-ragunathan
Copy link
Contributor

LGTM

@yongtang yongtang merged commit 88e36dc into moby:master Nov 30, 2017
@cpuguy83 cpuguy83 deleted the add_timeouts_to_splunk_post branch November 30, 2017 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants