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 plugin storage #864

Merged
merged 12 commits into from
Mar 29, 2016
Merged

Introduce plugin storage #864

merged 12 commits into from
Mar 29, 2016

Conversation

tagomoris
Copy link
Member

This patch is to add a new feature "plugin storages" to store key-value data to pluggable storages.
It is useful for many purposes:

  • re-implementation of pos file of file output plugin
  • store intermediate state of counter plugins (datacounter, flowcounter, ...) and make it resumable
  • make configurations dynamic and readable from external storages

This feature requires many more additions not included in this change for actual use cases (will be added in following changes):

  • simple storage path configuration with plugin id via system config
  • multi process support

@tagomoris
Copy link
Member Author

@repeatedly Could you review this?

@repeatedly
Copy link
Member

@sonots Could you review this first? I'm now busy so I don't have enough time to review large patch...

@tagomoris tagomoris force-pushed the introduce-plugin-storage branch 2 times, most recently from 7335c13 to 4e7170f Compare March 25, 2016 00:46
@sonots
Copy link
Member

sonots commented Mar 27, 2016

Comments

  • JSONStorage sounds like that we have to create new storage just for different formats. It looked FileStorage with json format is better design
    • Are you not assuming an extendability like S3Storage, RedisStorage, MySQLStorage? Extendability is only for formats?
    • RedisStorage should be useful to share values among multiple processes (on multiple hosts)
  • I want incr methods which allows us to atomically increment for counter plugins.

@tagomoris
Copy link
Member Author

@sonots File is not good abstraction, because S3 (and many other file systems) has files.
I considered some other naming options like LocalFileStorage, LocalStorage or others, but couldn't find something good enough. (Because json storage also works as on-memory storage if local disk path is not configured.)
I don't have any plan to make formats configurable for now.

@tagomoris
Copy link
Member Author

incr or such like operation will be supported by Counter API #858

@sonots
Copy link
Member

sonots commented Mar 28, 2016

LocalStorage

LocalStorage sounds good for me because it expresses it would store into local memory or local file, but not S3. At least, it should be better than JSONStorage because it sounds like it would store into S3 (we can not know the storage location from its name)

I don't have any plan to make formats configurable for now.

Okay, I think it is fine because I guess most of users not care of the format (unless they deeply care of performance)

incr

Okay, thanks.

@tagomoris tagomoris force-pushed the introduce-plugin-storage branch 2 times, most recently from 586dcad to 49d68b2 Compare March 28, 2016 04:50
@tagomoris
Copy link
Member Author

I changed default storage name from "json" to "local".

@sonots
Copy link
Member

sonots commented Mar 28, 2016

LGTM!

repeatedly
repeatedly reviewed Mar 28, 2016
View reviewed changes
lib/fluent/plugin/storage_local.rb
data = Yajl::Parser.parse(open(@path, 'r:utf-8'){ |io| io.read })
raise Fluent::ConfigError, "Invalid contents (not object) in plugin storage file: '#{@path}'" unless data.is_a?(Hash)
rescue => e
log.error "Failed to read data fron plugin storage file", path: @path, error_class: e.class, error: e
Copy link
Member

Choose a reason for hiding this comment

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

typo: fron

@tagomoris tagomoris force-pushed the introduce-plugin-storage branch from 49d68b2 to 4d29491 Compare March 29, 2016 05:52
@tagomoris
Copy link
Member Author

I pushed fixes for comments.

@repeatedly
Copy link
Member

LTGM

@tagomoris
Copy link
Member Author

Okay.

@tagomoris tagomoris merged commit 06f5a3e into master Mar 29, 2016
@tagomoris tagomoris deleted the introduce-plugin-storage 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.
3 participants
@tagomoris @repeatedly @sonots