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.

Sign up for GitHub

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

Jump to bottom

Introduce New Plugin Classes #866

Merged
merged 36 commits into from
Apr 19, 2016
Merged

Introduce New Plugin Classes #866

merged 36 commits into from
Apr 19, 2016

Conversation

tagomoris
Copy link
Member

New base classes for v0.14 new plugin APIs.
These are introduced to make these new features enabled:

  • Plugin Helpers
  • Plugin Storages
  • Async/Parallel buffer flushing & delayed commit
  • Buffer chunking with various data including time and tag (merging forest plugin)
  • New test drivers

@tagomoris tagomoris force-pushed the introduce-new-plugin-classes branch 2 times, most recently from bcfc08d to 3ac2264 Compare March 31, 2016 05:34
@tagomoris tagomoris force-pushed the introduce-new-plugin-classes branch from 56f70aa to cd4220b Compare April 5, 2016 12:46
@tagomoris
Copy link
Member Author

I've almost done for:

  • new base classes for input/output/buffer
  • new implementation for memory/file buffer plugin

I've not done yet for:

  • tests for new base classes and buffer plugins
  • optimization for many code
  • compatibility layers for old plugin
  • test drivers for new plugins

@sonots
Copy link
Member

sonots commented Apr 5, 2016

Feature is too generic, and I could not easily figure out what it means. It is better to think of better name, I think.

@sonots
Copy link
Member

sonots commented Apr 5, 2016

Metadata looks nice since I've ever implemented a code to parse tsuffix to get chunk key time in fluent-plugin-s3. It was ugly. 😄

@tagomoris
Copy link
Member Author

@sonots Please give me another options for Feature :(

@sonots
Copy link
Member

sonots commented Apr 6, 2016

What about StateModel? I first felt StateMachine would be good, but it looks it is not exactly StateMachine.

@tagomoris
Copy link
Member Author

I added a commit to solve Feature naming in other way.
0594ec6

@tagomoris
Copy link
Member Author

I cannot understand the difference between StateModel and StateMachine. And it's hidden thing even for plugin developers (plugin helper is retry_state and retry_state_create). So I want not to take long time for it...

@sonots
Copy link
Member

sonots commented Apr 6, 2016

I was not sure what you wanted to express with the word "Feature" because there was no explanation of it, but just "Base" looks fine for me :-)

@tagomoris tagomoris force-pushed the introduce-new-plugin-classes branch from e30b592 to c142e7e Compare April 7, 2016 04:56
repeatedly
repeatedly reviewed Apr 7, 2016
View reviewed changes
lib/fluent/config/configure_proxy.rb
@@ -89,6 +95,9 @@ def merge(other) # self is base class, other is subclass
if overwrite?(other, :alias)
raise ConfigError, "BUG: subclass cannot overwrite base class's config_section: alias"
end
if overwrite?(other, :configured_in_section)
raise ConfigError, "BUG: subclass cannot overwrite base class's config_section: configured_in"
Copy link
Member

Choose a reason for hiding this comment

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

configured_in_section instead of configured_in?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. This message is for authors of plugins, and plugins use configured_in method to specify this value.

@repeatedly
Copy link
Member

Could you write a rough image of buffer structure like this? http://docs.fluentd.org/images/buffer-internal-and-parameters.png

@tagomoris
Copy link
Member Author

@repeatedly for documentation? If so, I'll do it in later step.

@repeatedly
Copy link
Member

@tagomoris Mainly for documentation and good for reviwing your code.

@tagomoris tagomoris force-pushed the introduce-new-plugin-classes branch from b615cdd to eeb9e5c Compare April 18, 2016 08:59
@tagomoris tagomoris force-pushed the introduce-new-plugin-classes branch from eeb9e5c to 2c825e8 Compare April 18, 2016 09:10
@tagomoris tagomoris force-pushed the introduce-new-plugin-classes branch from b38f06a to 3a98938 Compare April 18, 2016 09:47
@tagomoris tagomoris force-pushed the introduce-new-plugin-classes branch from 2518dee to fe56d93 Compare April 19, 2016 02:05
@tagomoris
Copy link
Member Author

I'll merge this branch after CI green confirmation.

@cosmo0920
Copy link
Contributor

Travis CI reports green. But AppVeyor complains about dozens of errors and failures.

@sonots
Copy link
Member

sonots commented Apr 19, 2016

Just a question, are we going to publish with names such as file2 or memory2? Or, will they be changed?

@tagomoris
Copy link
Member Author

They will be changed to file and memory. Current names are just temporal one.

@tagomoris
Copy link
Member Author

I confirmed that CIs are now green.

@tagomoris tagomoris merged commit 1a3df5e into master Apr 19, 2016
@tagomoris
Copy link
Member Author

Merged!

@tagomoris tagomoris deleted the introduce-new-plugin-classes branch May 17, 2016 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tagomoris @sonots @repeatedly @cosmo0920 @Taher-Ghaleb