-
Notifications
You must be signed in to change notification settings - Fork 75
Use dynamodb for sincedb storage #55
base: master
Are you sure you want to change the base?
Conversation
New configuration parameter: dynamodb_sincedb_table : name of dynamodb table to use/create
This is awesome! I was just thinking of implementing this and now I don't have to. Looking forward to getting this merged in. |
@@ -158,6 +184,54 @@ def priority_of(group) | |||
@priority.index(group) || -1 | |||
end | |||
|
|||
public | |||
def create_ddb_table | |||
@logger.debug("Creating dynamodb table: #{@dynamodb_sincedb_table}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any previous-art on official logstash plugins using dynamo for state?
I see the value in supporting dynamo, but am not sure I like the auto-creation behaviour. I think I would prefer we error and boot, and require the user to correctly create the table.
We could provide documentation on the correct schema, but not doing it automatically would prevent any issues arising from misconfiguration which may cause it to go back and re-read histories, which could be unintended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not see anything else use DDB, so just went with what worked for us.
Regarding autocreation - thinking analogously: a sincedb filesystem file based record is also created automatically? I like autocreation because it makes my deployment simpler, especially as we may end up with quite a few of these.
However, I see your point too - perhaps we should have a configuration parameter which switches between autocreate and error behaviours?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with a configuration to change the behaviour - defaulting to auto-create will probably reduce number of issues logged, but also allow more control for the situation I was concerned about.
} | ||
} | ||
) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if we errored
here due to something like dynamo throttles? Should special handling be created for that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I should have put in some error checking here :)
Thanks for the contribution! It looks like the failing tests are due to some upstream changes to the I do have a couple concerns, but overall it looks like a good start. |
So, running in a container, I can't rely on a file system based sincedb - so I've written this to store sincedb in dynamodb instead. It needs suitable AWS permissions to create a table, read items and put items.
New configuration parameter:
dynamodb_sincedb_table : name of dynamodb table to use - it will be created if it doesn't already exist.
And if that parameter exists then it will ignore all the sincedb file, and use dynamodb
TODO: Need to work out how to do rspec tests on this, and the code probably needs some tidying
Thanks!