Skip to content

BI-1555 - Cache program ontology terms#206

Merged
timparsons merged 24 commits intofuture/1.0from
feature/BI-1555
Sep 27, 2022
Merged

BI-1555 - Cache program ontology terms#206
timparsons merged 24 commits intofuture/1.0from
feature/BI-1555

Conversation

@timparsons
Copy link
Member

@timparsons timparsons commented Aug 18, 2022

Description

Story: https://breedinginsight.atlassian.net/browse/BI-1555

  • Modified the underlying cache storage from in memory of the running application to now being stored in Redis.
  • Added the Redisson client library
  • Modified the ProgramCache to utilize Redis instead of Guava's LoadingCache.
  • Implemented caching for the TraitDAO

One set of changes that appears rather irrelevant to the story is with ProgramDAO and TraitDAO being refactored to interfaces, and then the creation of ProgramDAOImpl, TraitDAOImpl, and AbstractDAO. The main reason for this was when trying to mock the original ProgramDAO or TraitDAO, errors were thrown when creating the mocks. The fix for this (per Micronaut) is to have interfaces for things you'd like to mock, especially if the class being mocked extends another class. By creating an interface, mocking is much simpler, and doesn't run into any issues due to inheritance.

Dependencies

Redis container -> this has been added to the docker-compose.yml file in bi-api, so you should be able to start up that container, and the code will default to localhost:6379 to look for a Redis server to connect to.

Testing

Test traits:

  1. Start up bi-api and wait for a log message like cache loading complete for key: <programUUID>:trait (you may want to wait until you see your program UUID of interest)
  2. navigate to the ontology terms page, and ensure the traits load (hopefully faster)

Test germplasm:

  1. Start up bi-api and wait for a log message like cache loading complete for key: <programUUID>:brapigermplasm (you may want to wait until you see your program UUID of interest)
  2. navigate to the all germplasm page, and ensure the germplasm records load (hopefully faster)

Test restart (records should persist in redis between bi-api restarts):

  1. start bi-api and let the cache load
  2. restart bi-api, and refresh the germplasm or trait list page immediatly after the server successfully starts up
  3. ensure the page loads with all the appropriate records

Checklist:

  • I have performed a self-review of my own code
  • I have tested my code and ensured it meets the acceptance criteria of the story
  • I have tested that my code works with both the brapi-java-server and BreedBase
  • I have create/modified unit tests to cover this change
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to documentation
  • I have run TAF: <please include a link to TAF run> Need to merge changes for BI-1555 - Cache program ontology terms bi-docker-stack#20 before TAF can be run

@timparsons timparsons requested review from a team, HMS17 and davedrp and removed request for a team September 8, 2022 20:12
Copy link
Contributor

@HMS17 HMS17 left a comment

Choose a reason for hiding this comment

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

Testing locally revealed some problems.

Brapi:

  • Single trait add errors
  • Batch trait import errors
  • Batch germplasm import passes
  • Germplasm table display passes
  • Germplasm list table display passes
  • Germplasm export passes

BB:

  • First single trait add passes
  • Second single trait add "passes" but doesn't display
  • Batch trait import "passes" but doesn't display and neither does second single trait. Traits do not display after waiting and refreshing, but do display once bi-api is restarted
  • Batch germplasm import passes and germplasm displayed for two separate imports
  • Displaying germplasm lists errors

public class ProgramCacheUnitTest {
public class ProgramCacheUnitTest extends DatabaseTest {

// TODO: Tests
Copy link
Contributor

Choose a reason for hiding this comment

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

should this old todo be removed at this point?

.documentationURL(program.getDocumentationUrl());

// POST programs to each brapi service
// TODO: If there is a failure after the first brapi service, roll back all before the failure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this todo being implemented as part of this card, or another?

Copy link
Member Author

Choose a reason for hiding this comment

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

NA, was a pre-existing TODO


BrAPIProgram brApiProgram = brApiPrograms.getBody().getResult().getData().get(0);

//TODO: Need to add archived/not archived when available in brapi
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise re: is this a todo with a card associated, a todo for this card, or just something to do in future?

Copy link
Member Author

Choose a reason for hiding this comment

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

NA, was a pre-existing TODO

.build());
}

//TODO figure out why BrAPIServiceFilterIntegrationTest fails when requestTimeout is set in the constructor
Copy link
Contributor

Choose a reason for hiding this comment

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

A todo for this card?

Copy link
Member Author

Choose a reason for hiding this comment

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

NA, was a pre-existing TODO

return new ArrayList<>();
}

// Map<UUID, Trait> dbVariablesMap = dbVariables.stream().collect(Collectors.toMap(Trait::getId, p -> p));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this still be commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed


List<BrAPIObservationVariable> variables = searchVariables(ids);

// TODO: make sure have all expected external references
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO for this card?

Copy link
Member Author

Choose a reason for hiding this comment

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

NA, was a pre-existing TODO


List<BrAPIObservationVariable> variables = searchVariables(ids);

// TODO: make sure have all expected external references
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO for this card?

Copy link
Member Author

Choose a reason for hiding this comment

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

NA, was a pre-existing TODO

@Override
public List<Trait> createTraitsBrAPI(List<Trait> traits, User actingUser, Program program){

//TODO: Pass ontology reference
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO for this card?

Copy link
Member Author

Choose a reason for hiding this comment

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

NA, was a pre-existing TODO



// POST variables to each brapi service
// TODO: If there is a failure after the first brapi service, roll back all before the failure.
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO for this card?

Copy link
Member Author

Choose a reason for hiding this comment

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

NA, was a pre-existing TODO


@Override
public Trait updateTraitBrAPI(Trait trait, Program program) {
//TODO: Need to roll back somehow if there is an error
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO for this card?

Copy link
Member Author

Choose a reason for hiding this comment

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

NA, was a pre-existing TODO

@davedrp
Copy link
Contributor

davedrp commented Sep 14, 2022

Developer Testing failed -
Steps to reproduce

  1. viewed Ontology list (42-ontologies were present)
  2. Import 3-ontologies.
  3. clear browser cash
  4. viewed Ontology list

Expected result
Ontology list has 45-ontologies (The original 42 plus the 3 new ontologies).

Actual result
Only the original 42-ontologies were present. The 3 new ontologies were not present.

NOTE
if the bi-api server was restarted, and the Ontology list was viewed, the 3 new ontologies were not present.

Copy link
Contributor

@davedrp davedrp left a comment

Choose a reason for hiding this comment

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

developer-test failed

@timparsons timparsons requested review from HMS17 and davedrp September 15, 2022 14:15
@davedrp
Copy link
Contributor

davedrp commented Sep 15, 2022

passed my Developer testing

@timparsons timparsons changed the base branch from develop to future/1.0 September 15, 2022 18:34
Copy link
Contributor

@HMS17 HMS17 left a comment

Choose a reason for hiding this comment

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

Tests passed!

Brapi:

  • Single trait add passes
  • Batch trait import passes
  • Batch germplasm import passes
  • Germplasm table display passes
  • Germplasm list table display passes
  • Germplasm export passes

BB:

  • First single trait add passes
  • Second single trait add passes
  • Batch trait import passes
  • Batch germplasm import passes and germplasm displayed for two imports
  • Displaying germplasm lists errors - but error also shows up in qa-test

@timparsons timparsons merged commit 4633f39 into future/1.0 Sep 27, 2022
@timparsons timparsons deleted the feature/BI-1555 branch September 27, 2022 20:49
timparsons added a commit that referenced this pull request Sep 29, 2022
timparsons added a commit that referenced this pull request Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants