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

Add suspend option to in dummy plugin #900

Merged

Conversation

cosmo0920
Copy link
Contributor

No description provided.

@cosmo0920 cosmo0920 changed the title [WIPAdd suspend option to in dummy plugin [WIP] Add suspend option to in dummy plugin Apr 15, 2016
@cosmo0920 cosmo0920 force-pushed the add-suspend-option-to-in_dummy-plugin branch 2 times, most recently from 99d8626 to 6c138c9 Compare April 22, 2016 01:20
@cosmo0920 cosmo0920 changed the title [WIP] Add suspend option to in dummy plugin Add suspend option to in dummy plugin Apr 22, 2016
@cosmo0920 cosmo0920 force-pushed the add-suspend-option-to-in_dummy-plugin branch 2 times, most recently from 78e2e69 to d87934e Compare May 31, 2016 05:56
@tagomoris
Copy link
Member

Please leave storage_local as is, because these code will be fixed once more with plugin id auto-configuration (with root path by system config)

@tagomoris
Copy link
Member

And, could you rebase changes of in_dummy on current master?

@cosmo0920 cosmo0920 force-pushed the add-suspend-option-to-in_dummy-plugin branch from d87934e to 0801089 Compare June 16, 2016 06:53
@cosmo0920
Copy link
Contributor Author

cosmo0920 commented Jun 16, 2016

I've rebased with current master.
But suspend option related tests didn't pass yet.

@cosmo0920 cosmo0920 force-pushed the add-suspend-option-to-in_dummy-plugin branch 4 times, most recently from 0c616dc to 1543ce4 Compare June 17, 2016 04:50
@cosmo0920
Copy link
Contributor Author

Please leave storage_local as is, because these code will be fixed once more with plugin id auto-configuration (with root path by system config)

Oh.... I think almost storage_local code should be leave as-is. But this line https://github.com/fluent/fluentd/pull/900/files#diff-0380de12f73675c40904702aab31701cR61 should be fixed because without this fix I always got the following error:

================================================================================
Error: test: value of auto increment key is suspended after stop-and-start(DummyTest::suspend internal counters if suspend is true): Fluent::ConfigError: Directory is not writable for plugin storage file 'test/tmp/in_dummy/store/json'
/home/hhatake/Github/fluentd/lib/fluent/plugin/storage_local.rb:61:in `configure'
/home/hhatake/Github/fluentd/lib/fluent/plugin_helper/storage.rb:92:in `block in configure'
/home/hhatake/Github/fluentd/lib/fluent/plugin_helper/storage.rb:87:in `each'
/home/hhatake/Github/fluentd/lib/fluent/plugin_helper/storage.rb:87:in `configure'
/home/hhatake/Github/fluentd/lib/fluent/plugin/in_dummy.rb:61:in `configure'
/home/hhatake/Github/fluentd/lib/fluent/test/driver/base.rb:78:in `configure'
test/plugin/test_in_dummy.rb:12:in `create_driver'
test/plugin/test_in_dummy.rb:163:in `block (2 levels) in <class:DummyTest>'
================================================================================

@tagomoris
Copy link
Member

It's time to revoke this thread, and there is just a point to be fixed after many fixes during pending time.
@cosmo0920 Could you see my review comment (following this comment)?

@@ -58,12 +60,16 @@ def initialize
def configure(conf)
super
@dummy_index = 0
config = conf.elements.select{|e| e.name == 'storage' }.first
@storage = storage_create(usage: 'suspend', conf: config, type: :local)
Copy link
Member

Choose a reason for hiding this comment

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

Rebase this tree on master, and please use default_type instead of type keyword argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll try your suggested changes within a week.

@cosmo0920 cosmo0920 force-pushed the add-suspend-option-to-in_dummy-plugin branch from 936c60f to dcdebeb Compare August 5, 2016 08:38
@tagomoris
Copy link
Member

LGTM!

@tagomoris tagomoris merged commit a97ef73 into fluent:master Aug 8, 2016
@cosmo0920
Copy link
Contributor Author

Thanks!!!

@cosmo0920 cosmo0920 deleted the add-suspend-option-to-in_dummy-plugin branch August 8, 2016 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants