add batching to Copilot::Sync

[utako]

• A short explanation of the proposed change:

  Add batching to Copilot::Sync per our discussion with @tcdowney and @cwlbraa

  • Copilot::Sync only batches queries to the database.
  • It does not send batched resources to Copilot. Sending batched resources will be a bigger change and will be considered in the future.

[#161293119]

• An explanation of the use cases your change solves

  When there are hella routes, route mappings and processes, database queries should be less expensive

• Links to any other associated PRs
  Check for nil entries in bulk sync call #1227

• I have reviewed the contributing guide

• I have viewed, signed, and submitted the Contributor License Agreement

• I have made this pull request to the master branch

• I have run all the unit tests using bundle exec rake

• I have run CF Acceptance Tests

[cfdreddbot]

Hey utako!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/161301937

The labels on this github issue will be updated when the story is started.

[infews]

Did you take a look at the SequelPaginator? Sequel's datasets allow walking a query in pages/batches like you built up here.

[utako]

Did you take a look at the SequelPaginator? Sequel's datasets allow walking a query in pages/batches like you built up here.

@infews No, but that looks appropriate. We wanted to adhere to the same querying pattern that was built in the Sync queries for tasks and LRPs. We could submit another PR to convert this and the rest of the batched queries to use the SequelPaginator for consistency. Happy for CAPI to do it, too. 😄

[infews]

@tcdowney What do you think about @utako 's comment about query consistency?

[charleshansen]

tldr, we could maybe make improvements, but I think we should merge the PR.

I looked at it again, you wouldn't actually use the SequelPaginator itself, but the query would be simpler to read if you used the Sequel pagination extension, like https://github.com/cloudfoundry/cloud_controller_ng/blob/master/lib/cloud_controller/paging/sequel_paginator.rb#L12. On the other hand, it does make sense for the syncers to use the same batching strategy, as Utako said.

At this point, the benefit of any further refactoring is questionable and we should merge the PR unless there are other objections (I'm cool with the Proc usage at this point). If we decide we want to use sequel for pagination, we should do it everywhere and not just right here. That larger question can be discussed outside of this PR.

Waiting for a +1 before actually merging...

[tcdowney]

+1 to what @charleshansen said above. I think the Sequel Paginator suggestion that @infews had might be something we (CAPI) look into using across the sync jobs eventually, but not something I'd want to block this PR on (or would expect the Routing team to take on).

Looks good to me!