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

Allow tracking by column number, not just time #108

Closed
wants to merge 1 commit into from

Conversation

untergeek
Copy link
Contributor

closes #57

@talevy
Copy link
Contributor

talevy commented Jan 5, 2016

we should not remove the tracking of sql_last_start. instead, we should allow for keeping track of both. Some queries will probably want to filter on both a specific tracking_column and sql_last_start.

@untergeek
Copy link
Contributor Author

You think so? I'm guess that's a possibility, but it seems like a rather uncommon one.

@logger.debug? and @logger.debug("Executing JDBC query", :statement => statement, :parameters => parameters, :count => query.count)

if @jdbc_paging_enabled
query.each_page(@jdbc_page_size) do |paged_dataset|
paged_dataset.each do |row|
sql_last_value = row[@tracking_column.to_sym] if @use_column_value
Copy link
Contributor

Choose a reason for hiding this comment

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

should we log a warning if @tracking_column is not present in the result set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably a good idea.

@@ -175,17 +179,20 @@ def execute_statement(statement, parameters)
begin
parameters = symbolized_params(parameters)
query = @database[statement, parameters]
sql_last_start = Time.now.utc
sql_last_value = @use_column_value ? @sql_last_value : Time.now.utc
@warning_sent = false
Copy link
Contributor

Choose a reason for hiding this comment

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

since we may introduce additional warnings, maybe this should be named something specific to the column retrieval?

@talevy
Copy link
Contributor

talevy commented Jan 6, 2016

left a few comments, looks good.

Test for tracking_column and warn user once per query if it's not there.
Add a test to verify this is working properly

closes logstash-plugins#57
@elasticsearch-bot
Copy link

Aaron Mildenstein merged this into the following branches!

Branch Commits
master 5c495b4

@leifurhauks
Copy link

When using a tracking column, if the statement retrieves multiple rows, which value will be saved? The maximum value? Or the value from the "last" row in the result set?

@talevy
Copy link
Contributor

talevy commented Jan 25, 2016

@leifurhauks as per the current implementation[1], it is the value from the "last" row in the result-set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

latest max column_name instead of sql_last_start
4 participants