Skip to content

Bump fluent/fluent-logger-golang v1.4.0#39074

Merged
yongtang merged 2 commits intomoby:masterfrom
thaJeztah:bump_fluentd
Apr 19, 2019
Merged

Bump fluent/fluent-logger-golang v1.4.0#39074
yongtang merged 2 commits intomoby:masterfrom
thaJeztah:bump_fluentd

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 15, 2019

  • Add RequestAck to enable at-least-once message transferring
  • Add Async option to update sending message in asynchronous way
  • Deprecate AsyncConnect (Use Async instead)

full diff: fluent/fluent-logger-golang@v1.3.0...v1.4.0

Also updating tinylib/msgp v1.1.0

full diff: tinylib/msgp@3b556c6...v1.1.0

@thaJeztah
Copy link
Member Author

thaJeztah commented Apr 15, 2019

ping @tagomoris ptal 🤗

@thaJeztah
Copy link
Member Author

thaJeztah commented Apr 15, 2019

Some flakiness in docker-py tests;

02:22:31 _______________________ ExecTest.test_exec_command_demux _______________________
02:22:31 /docker-py/tests/integration/api_exec_test.py:182: in test_exec_command_demux
02:22:31     assert next(exec_log) == (b'hello out\r\n', None)
02:22:31 E   AssertionError: assert ('hello out\r...rr\r\n', None) == ('hello out\r\n', None)
02:22:31 E     At index 0 diff: 'hello out\r\nhello err\r\n' != 'hello out\r\n'
02:22:31 E     Use -v to get the full diff
02:22:31 - generated xml file: /go/src/github.com/docker/docker/bundles/test-docker-py/results.xml -
02:22:31 =========================== short test summary info ============================

@codecov
Copy link

codecov bot commented Apr 15, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@ad9362b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #39074   +/-   ##
=========================================
  Coverage          ?   36.98%           
=========================================
  Files             ?      612           
  Lines             ?    45390           
  Branches          ?        0           
=========================================
  Hits              ?    16788           
  Misses            ?    26315           
  Partials          ?     2287

cpuguy83
cpuguy83 approved these changes Apr 15, 2019
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

tagomoris
tagomoris reviewed Apr 16, 2019
View reviewed changes
Copy link
Contributor

@tagomoris tagomoris left a comment

Choose a reason for hiding this comment

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

I couldn't find any code to set RequestAck.
Does this change include it?

daemon/logger/fluentd/fluentd.go Outdated
Copy link
Contributor

@tagomoris tagomoris Apr 16, 2019

Choose a reason for hiding this comment

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

IMO it's better to do:

  • add asyncKey (fluentd-async) to enable async option
  • warn that asyncConnectKey is now not recommended and will be removed later, and also enable async if asyncConnectKey is specified (and unless asyncKey is disabled explicitly)

Copy link
Member Author

@thaJeztah thaJeztah Apr 16, 2019

Choose a reason for hiding this comment

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

Ah, right; my main goal with this PR was to keep the dependency up-to-date with upstream fixes, without changing the UX / options.

I'd be ok with adding a fluentd-async and marking fluentd-async-connect as deprecated (and perhaps adding an error or warning if both are specified?)

Probably better for a follow-up 🤗

Copy link
Member Author

@thaJeztah thaJeztah Apr 16, 2019

Choose a reason for hiding this comment

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

Let me rename the variable back in this PR though

@thaJeztah
Copy link
Member Author

thaJeztah commented Apr 16, 2019

I couldn't find any code to set RequestAck. Does this change include it?

No it doesn't (see my other comment #39074 (comment)); I can add it in that follow-up (it's a boolean opt, right?)

@thaJeztah
bump fluent/fluent-logger-golang v1.4.0
688e67e 
- Add RequestAck to enable at-least-once message transferring
- Add Async option to update sending message in asynchronous way
- Deprecate AsyncConnect (Use Async instead)

full diff: fluent/fluent-logger-golang@v1.3.0...v1.4.0

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
bump tinylib/msgp v1.1.0
048bd8d 
full diff: tinylib/msgp@3b556c6...v1.1.0

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah mentioned this pull request Apr 16, 2019
Merged
@tagomoris
Copy link
Contributor

tagomoris commented Apr 16, 2019

I see. LGTM.

@thaJeztah
Copy link
Member Author

thaJeztah commented Apr 16, 2019

Opened a follow-up PR with user-facing changes and deprecation in #39086

yongtang
yongtang approved these changes Apr 19, 2019
View reviewed changes
Copy link
Member

@yongtang yongtang left a comment

Choose a reason for hiding this comment

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

LGTM

@yongtang yongtang merged commit 97c25f6 into moby:master Apr 19, 2019
@thaJeztah thaJeztah deleted the bump_fluentd branch April 19, 2019 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

+2 more reviewers

@tagomoris tagomoris tagomoris left review comments

@yongtang yongtang yongtang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/logging status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thaJeztah @tagomoris @cpuguy83 @yongtang @GordonTheTurtle