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

add/delete only changed overlap records #132

Merged
merged 1 commit into from
Sep 28, 2021
Merged

Conversation

jsteverman
Copy link
Contributor

  • moves ETAS database ops to Module ETAS

@jsteverman jsteverman requested a review from aelkiss September 16, 2021 21:06
Copy link
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

I think the split into OverlapTableUpdate and ClusterUpdate makes sense. I think we should try to come up with a better name for the module they're in (admittedly not my forte).

I'll separately comment with thoughts on a test to add to make sure we're doing what we think we are regarding minimizing churn.

lib/etas/overlap_table_update.rb Outdated Show resolved Hide resolved
bin/etas/update_overlap_table.rb Outdated Show resolved Hide resolved
lib/etas/cluster_update.rb Outdated Show resolved Hide resolved
@aelkiss
Copy link
Member

aelkiss commented Sep 17, 2021

Regarding testing that minimizing churn is working as expected:

The specs are a little bit hard to follow but I think are basically already doing this by checking that the delete count is smaller than the old overlap count & likewise that the add count is smaller than the new overlap count.

You could consider adding a more explicit test like:

describe "#deletes" do
  it "does not include things in both the old & new overlaps" do
    do_some_setup
    expect(deletes).not_to contain(thing_that_shouldnt_be_deleted)
  end
end

describe "#adds" do
  it "does not include things in both the old & new overlaps" do
    do_some_setup
    expect(adds).not_to contain(thing_that_shouldnt_need_to_be_re_added)
  end
end

Regarding testing that it's actually reducing churn in mysql, probably the way to do that would be to stub the overlap table and then check that it receives the expected number of insert & delete calls:

it "doesn't churn" do
  mock_overlap_table = stub(:overlap_table)
  allow(mock_overlap_table).to receive(:filter).and_return(mock_overlap_table)
  expect(mock_overlap_table).to receive(:delete).exactly(M).times
  expect(mock_overlap_table).to receive(:insert).exactly(N).times  

  do_some_setup
  described_class.new(mock_overlap_table, cluster).upsert
end

You could I suppose set an expectation that filter or insert receives the parameters you expect it to; that's probably overkill since we're already checking the actual contents of the table. Doing it this way just allows us to make sure that we aren't calling insert or delete more than we expected.

Copy link
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Should be good to go once you resolve the merge conflict & squash.

spec/cluster_update_spec.rb Outdated Show resolved Hide resolved
 * only updates records that have been changed
 * avoids use of constants
 * does NOT move ETAS database ops to Module ETAS
 * debug log cluster._id
@jsteverman jsteverman merged commit 0727739 into master Sep 28, 2021
@aelkiss aelkiss deleted the HT-3026_etas_update branch October 1, 2021 14:50
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.

3 participants