Skip to content
This repository was archived by the owner on Nov 5, 2018. It is now read-only.

Conversation

@seg10
Copy link
Contributor

@seg10 seg10 commented Sep 4, 2018

Previously, the first query to couchdb would take a hit on total time. This in turn will skew the various load/performance tests that we're running at the infrastructure level.

@mrtyler
Copy link
Contributor

mrtyler commented Sep 4, 2018

This approach seems fine. Couple questions:

  1. @klown has some in-flight changes in GPII-3138: Update Prefs Safes and their GPII Keys in the data base #6. We'll need coordination between these two PRs.

  2. Have you thought about the coupling between the list of indices here and wherever those indices are created/managed (probably somewhere in universal)? Can we ensure these two sources of truth stay in sync?

loadData.sh Outdated
warm(){
VIEW=$1

curl -fsS $COUCHDB_URL/_design/views/_view/$VIEW >/dev/null

Choose a reason for hiding this comment

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

This is very expensive query, since it executes view function against ALL data in DB. I think this works now only because we have small number of records.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One option to reduce the cost of this query might be to run it immediately after the creation of the view when. This would require a higher degree of coupling with the test data however and I'm not sure I like the tradeoff considering that load data on a new DB should be pretty infrequent.

@stepanstipl
Copy link

Apart from Sergey's comments, it looks good. Did you have a chance to test this all together and that it solves the GPII-3250 when a new cluster is deployed?

Sergio added 2 commits September 5, 2018 08:23
This should reduce the amount of additional maintenance in the future
@seg10
Copy link
Contributor Author

seg10 commented Sep 5, 2018

@mrtyler - thanks for your comments. Responses to your question below:

@klown has some in-flight changes in #6. We'll need coordination between these two PRs.

The changes on this PR are small in comparison to the structural changes @klown is making in his PR. Do we have any idea on the time frames for his merge? If it's too far off, I'd rather that we go ahead and merge this one and work on rebasing his branch after before his gets merged.

Have you thought about the coupling between the list of indices here and wherever those indices are created/managed (probably somewhere in universal)? Can we ensure these two sources of truth stay in sync?

The latest commits should address this as we're now dynamically querying the views that are available.

@seg10
Copy link
Contributor Author

seg10 commented Sep 5, 2018

@stepanstipl - As far as testing goes:

Apart from Sergey's comments, it looks good. Did you have a chance to test this all together and that it solves the GPII-3250 when a new cluster is deployed?

My manual testing strategy has been to:

  1. Delete the gpii couchdb database.
  2. Delete the couchdb pods. The statefulSet will recreate them.
  3. Spin up a new pod using kubectl run -it ... with the old dataLoader image.
  4. Copy over my local, modified dataLoader.sh script into this container.
  5. Execute the new dataLoader.sh script
  6. Measure the time for the first query against preferences server using curl.

In repeated tests, if the old dataLoader.sh script was executed in step (5), then the first query in step (6) would take around 4-5 seconds. If the new dataLoader.sh script is executed in step (5), then step (6) would take less than half a second.

Note that in my experimentation, step (2) is necessary for couchdb to clear the index. For some reason, couchdb seems to keep the index in some form even if the db is deleted in (1).

@klown
Copy link

klown commented Sep 5, 2018

@seg10, I don't know if you heard during today's architecture meeting, but I replied about coordinating just as the meeting was ended. I'm "clown" on IRC in #fluid-work or #fluid-tech.

@klown
Copy link

klown commented Sep 5, 2018

@seg10, You wrote:

The changes on this PR are small in comparison to the structural changes @klown is making in his PR. Do we have any idea on the time frames for his merge? If it's too far off, I'd rather that we go ahead and merge this one and work on rebasing his branch after before his gets merged.

My changes to gpii-dataloader depend on changes in universal and changes in gpii-infra, and they all have to go in together. I estimate about two weeks before that happens. If your changes are ready, then I agree that it makes sense to merge this PR and then rebase my branch.

@mrtyler
Copy link
Contributor

mrtyler commented Sep 5, 2018

LGTM

Sounds like you and Joseph have a plan for merging things. I like querying the available views and warming them all -- no coupling at all!

@natarajaya
Copy link

I tested this with my dev cluster and seems like this approach is working.
I still have concerns regarding warm-up function that is being used.
But I guess it is fine for now and it is definitely better than no warm-up at all, so LGTM!

@stepanstipl
Copy link

Cool, LGTM

@seg10
Copy link
Contributor Author

seg10 commented Sep 6, 2018

Thanks for all the reviews! Will merge and close this out then.

@seg10 seg10 merged commit f28bbfd into gpii-ops:master Sep 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants