Support compress in forward plugin #1179

ganmacs · 2016-08-24T06:17:32Z

Because this PR depends on #1172, please merge this after merging #1172.

This PR closes #504.

tagomoris · 2016-08-26T07:43:39Z

lib/fluent/event.rb

+      @compressed_data = data
+    end
+
+    def dup


This re-implementation looks no need.

Ah... It's better to fix original implementation MessagePackEventStream#dup to use self.class instead of re-implementing this method in subclass.

tagomoris · 2016-08-26T07:55:57Z

I added review comments on code.

ganmacs · 2016-08-29T06:56:46Z

@tagomoris
I updated.

ganmacs · 2016-08-31T09:28:36Z

@tagomoris

I rebased and updated code.

tagomoris · 2016-09-01T06:34:52Z

lib/fluent/event.rb

+    end
+
+    def each(&block)
+      ensure_decompressed! unless @unpacked_times


Unnecessary unless

tagomoris · 2016-09-07T02:04:46Z

@ganmacs could you rebase/update your change?

it contain compressd message pack

ganmacs · 2016-09-08T04:28:56Z

@tagomoris
Rebased 👌

tagomoris · 2016-09-12T04:40:10Z

test/plugin/test_in_forward.rb

+      entries = ''
+      events.each do |_tag, _time, record|
+        v = ''
+        [_time, record].to_msgpack(v)


It's already deprecated feature to assign String objects as an argument of #to_msgpack.
Please update the code as v << [_time, record].to_msgpack.

and use event_time

ganmacs · 2016-09-12T06:41:07Z

@tagomoris
Updated. Could you re-review this change?

tagomoris · 2016-09-12T07:05:55Z

test/test_event.rb

+      @times.zip(@records).each do |_time, record|
+        v = ''
+        v << [_time, record].to_msgpack
+        @packed_record += v


@packed_record << [_time, record].to_msgpack looks to work well, and it's simple.

oops😨
Thanks. Fixed it.

and add test to ensure call log.info

tagomoris · 2016-09-12T08:12:50Z

I removed my LGTM, because I found the bug and missing tests, from warning of test results.

/home/travis/build/fluent/fluentd/lib/fluent/plugin/out_forward.rb:444: warning: instance variable @compress not initialized
/home/travis/build/fluent/fluentd/lib/fluent/plugin/out_forward.rb:452: warning: instance variable @compress not initialized

This warning shows that Node instances doesn't have instance variable named as @compress.
Your patch doesn't feed @compress of output plugin into ForwardOutput::Node objects.
So compression transferring is always false in this code.
And there are no tests about it.

ganmacs · 2016-09-12T08:53:35Z

@tagomoris
I fixed a bug that compress option is always :text and added some tests.

tagomoris · 2016-09-12T09:08:09Z

@ganmacs It's ok, but not enough. It's needed to check whether in_forward (instance of target_input_driver) creates CompressedMessagePackEventStream or not.

ganmacs · 2016-09-13T02:44:15Z

@tagomoris I added a test that in_forward creates CompressedMessagePackEventStream or not.

tagomoris · 2016-09-13T04:30:43Z

LGTM. Thank you for great patch!

* Add CompressedMessagePackEventStream class it contain compressd message pack * Use CompressedMessagePackEventStream to handle compressed data * Add compress config_params to out_forward * Change compressed to ensure_compress! * Not to enable compress forcedly * `compress` -> `compressed` * Fix failed test and use event_time * Remove deprecated methods * Change test description and add test to ensure call log.info * Add tests to check `ensure_decompressed` is called * Fix a bug that `Node` doesn't have compress type * Add a test that `in_forward` creates CompressedMessagePackEventStream

repeatedly mentioned this pull request Aug 25, 2016

Support compression in forward transfer #637

Closed

ganmacs force-pushed the support-compress-in-forward-plugin branch 2 times, most recently from 44edf57 to 38fc6ac Compare August 26, 2016 05:11

ganmacs changed the title ~~[WIP] Support compress in forward plugin~~ Support compress in forward plugin Aug 26, 2016

tagomoris reviewed Aug 26, 2016
View reviewed changes

tagomoris added the v0.14 label Aug 27, 2016

ganmacs force-pushed the support-compress-in-forward-plugin branch 5 times, most recently from 9e387f2 to 04d1366 Compare August 31, 2016 09:25

ganmacs force-pushed the support-compress-in-forward-plugin branch 2 times, most recently from 956676f to 18e530f Compare September 1, 2016 02:13

tagomoris reviewed Sep 1, 2016
View reviewed changes

lib/fluent/event.rb

end

def each(&block)

ensure_decompressed! unless @unpacked_times

Copy link

Member

tagomoris Sep 1, 2016

Choose a reason for hiding this comment

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

Unnecessary unless

ganmacs force-pushed the support-compress-in-forward-plugin branch from 18e530f to 3808249 Compare September 1, 2016 07:56

This was referenced Sep 6, 2016

Support a data compression in buffer plugins #1169

Closed

support the compressing stream in forward and secure forward #504

Closed

ganmacs added 4 commits September 8, 2016 10:20

Add CompressedMessagePackEventStream class

66be0ce

it contain compressd message pack

Use CompressedMessagePackEventStream to handle compressed data

74c0e02

Add compress config_params to out_forward

5b70bcc

Change compressed to ensure_compress!

8aacdd5

tagomoris reviewed Sep 12, 2016
View reviewed changes

Fix failed test

d675e46

and use event_time

ganmacs force-pushed the support-compress-in-forward-plugin branch 2 times, most recently from b766cb7 to e1bc305 Compare September 12, 2016 06:39

tagomoris reviewed Sep 12, 2016
View reviewed changes

ganmacs added 3 commits September 12, 2016 16:39

Remove deprecated methods

91dd964

Change test description

a2d0719

and add test to ensure call log.info

Add tests to check ensure_decompressed is called

6535ef9

ganmacs force-pushed the support-compress-in-forward-plugin branch from e1bc305 to 6535ef9 Compare September 12, 2016 07:41

Fix a bug that Node doesn't have compress type

80addd2

ganmacs force-pushed the support-compress-in-forward-plugin branch from f1ed8a8 to 80addd2 Compare September 12, 2016 08:59

Add a test that in_forward creates CompressedMessagePackEventStream

60381fa

ganmacs force-pushed the support-compress-in-forward-plugin branch from 4642699 to 60381fa Compare September 13, 2016 03:14

tagomoris merged commit 335d3b2 into fluent:master Sep 13, 2016

ganmacs deleted the support-compress-in-forward-plugin branch November 28, 2019 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support compress in forward plugin #1179

Support compress in forward plugin #1179

ganmacs commented Aug 24, 2016

tagomoris Aug 26, 2016

tagomoris Aug 26, 2016

tagomoris commented Aug 26, 2016

ganmacs commented Aug 29, 2016

ganmacs commented Aug 31, 2016

tagomoris Sep 1, 2016

tagomoris commented Sep 7, 2016

ganmacs commented Sep 8, 2016

tagomoris Sep 12, 2016

ganmacs commented Sep 12, 2016

tagomoris Sep 12, 2016

ganmacs Sep 12, 2016

tagomoris commented Sep 12, 2016

ganmacs commented Sep 12, 2016

tagomoris commented Sep 12, 2016

ganmacs commented Sep 13, 2016

tagomoris commented Sep 13, 2016

Support compress in forward plugin #1179

Support compress in forward plugin #1179

Conversation

ganmacs commented Aug 24, 2016

tagomoris Aug 26, 2016

Choose a reason for hiding this comment

tagomoris Aug 26, 2016

Choose a reason for hiding this comment

tagomoris commented Aug 26, 2016

ganmacs commented Aug 29, 2016

ganmacs commented Aug 31, 2016

tagomoris Sep 1, 2016

Choose a reason for hiding this comment

tagomoris commented Sep 7, 2016

ganmacs commented Sep 8, 2016

tagomoris Sep 12, 2016

Choose a reason for hiding this comment

ganmacs commented Sep 12, 2016

tagomoris Sep 12, 2016

Choose a reason for hiding this comment

ganmacs Sep 12, 2016

Choose a reason for hiding this comment

tagomoris commented Sep 12, 2016

ganmacs commented Sep 12, 2016

tagomoris commented Sep 12, 2016

ganmacs commented Sep 13, 2016

tagomoris commented Sep 13, 2016