Add transparent support for count query embargo on failure. #294
Conversation
|
Changelog and gemspec updates to follow. |
| @@ -0,0 +1,35 @@ | |||
|
|
|||
| module LogStash module PluginMixins | |||
There was a problem hiding this comment.
IMO we should not place plugin specific (or integration specific) mixins in LogStash::PluginMixins, all of these could be namespaced to LogStash::PluginMixins::Jdbc or similar
There was a problem hiding this comment.
Agreed. This is old code but I can fix it now.
| @@ -0,0 +1,35 @@ | |||
|
|
|||
| begin | ||
| execute_count(query) | ||
| @count_is_supported = true | ||
| rescue Exception => e |
There was a problem hiding this comment.
is there a risk that the query fails here for an arbitrary reason not related to counts?
There was a problem hiding this comment.
I suppose so. I'll see if I can find a more specific Exception to rescue.
| return unless @in_debug | ||
| check_count_query(query) if @needs_check | ||
| if @count_is_supported | ||
| @logger.debug("Executing JDBC query", :statement => statement, :parameters => parameters, :count => query.count) |
There was a problem hiding this comment.
since execute_count was implemented to do query.count, maybe we use it here as well?
spec/inputs/jdbc_spec.rb
Outdated
| expect(logger).to receive(:debug).once.with("Executing JDBC query", :statement => settings["statement"], :parameters => {:sql_last_value=>"bar"}) | ||
| plugin.run(queue) | ||
| event = queue.pop | ||
| expect(event.get("num")).to eq(1) |
There was a problem hiding this comment.
this test is about not debugging with a count key, maybe we can remove the remaining assertions and create a separate test to check the presence of things that we expect?
There was a problem hiding this comment.
Sure. I can remove the other expectations, as this context block is a copy of the one at line 1240, those expectations have been verified already.
OTOH, it could be argued that it is important to verify that the normal event creation still occurs when the code from this PR is in the call path and when some degree of stubbing is used.
There was a problem hiding this comment.
Agreed it may be desirable to test that but I'd put it into a separate it block that uses the same test setup (thus it is the same scenario we're testing)
|
Changelog and gemspec updates made. Is this PR OK now? |
Many folks get errors when using DB drivers that are not supported by adapters in Sequel.
This update traps any errors when doing the first count query and prevents further count queries from being executed.