TRUNK-5362: Issue with method in EncounterService causing extreme slowness

[kdaud]

Description of what I changed

The search indexing criteria for getEncountersByPatientIdentifier() searches across all aspects of the patient (patient name, patient identifier, patient attributes), and this is what makes it extremely slow. The method should only search based upon what the method name states...by identifier.

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-5362

Checklist: I completed these to help reviewers :)

• [x ] My IDE is configured to follow the code style of this project.

  No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

• [ ] I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)

  No? -> write tests and add them to this commit git add . && git commit --amend

• [x ] I ran mvn clean package right before creating this pull request and added all formatting changes to my commit.

  No? -> execute above command

• [ ] All new and existing tests passed.

  No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

• [x ] My pull request is based on the latest changes of the master branch.

  No? Unsure? -> execute command git pull --rebase upstream master

[jwnasambu]

@kdaud kindly include the branch name to your ticket Id. This is what is expected TRUNK-5362:Issue with method in EncounterService causing extreme slowness

[jwnasambu]

@kdaud Oh sorry! you forgot to give the description of what you changed on this ticket.

[kdaud]

Thanks @jwnasambu ! Have done the needful!

[sjmckee]

I still don't believe this fixes the issue. Whether the identifier is passed as the name parameter or the identifier, the code in PatientServiceImpl picks one or the other and passes it to the DAO (name first if it's not null, identifier second). This change will make no difference in the behavior.

` public List<Patient> getPatients(String name, String identifier, List<PatientIdentifierType> identifierTypes, boolean matchIdentifierExactly, Integer start, Integer length) throws APIException {

	if(identifierTypes == null) {
		return dao.getPatients(name != null ? name : identifier, start, length);
	}
	else {
		return dao.getPatients(name != null ? name : identifier, identifierTypes, matchIdentifierExactly, start, length);
	}
}`

[kdaud]

Thanks @sjmckee for the highlight, initially I had thought turning the boolean value of matchIdentifierExactly to true would fix the issue.

I get to see that the method being used by the search indexing criteria in getEncountersByPatientIdentifier() is

public List<Patient> getPatients(String name, String identifier, List<PatientIdentifierType> identifierTypes,
    boolean matchIdentifierExactly) throws APIException {
		
    return Context.getPatientService().getPatients(name, identifier, identifierTypes, matchIdentifierExactly, 0, null);
}

[sjmckee]

Do you know if the number of queries are reduced after making the change? Sorry, I was hoping to have more time to devote to this.

[kdaud]

Not so certain!

But looking at the current boolean value for matchIdentifierExactly makes me think that the query is done against different parameters other than the one passed in the method. I thought setting the value to true would make it possible to query only the passed parameter.

[kdaud]

I found out the implementation of the method

public List<Patient> getPatients(String parameter) throws APIException {
    return Context.getPatientService().getPatients(parameter, 0, null);
}

only queries the passed parameter(patient_name, or patient_identifier, or patient_id, identifierType) hence one query is performed every time HibernatePatientDAO.getPatients(String query, boolean includeVoided, Integer start, Integer length) is executed. So have switched EncounterServiceImpl.getEncountersByPatientIdentifier() to instead use the method above.

cc: @sjmckee

[sjmckee]

That change still trickles down to the public List<Patient> findPatients(String query, boolean includeVoided, Integer start, Integer length) method in HibernatePatientDAO.java, and that method queries by identifier, then name, and then by person attribute. We only want to query by patient identifier.

[kdaud]

Seems like the method appears to be very fast until the database contains a fair amount of data. What am failing to understand is that why would HibernatePatientDAO.getPatients(String query, boolean includeVoided, Integer start, Integer length) query other parameters which have not been passed in the method? cc: @sjmckee

[sjmckee]

Yes. We had around 50,00 patients in our database, and it was extremely slow. Here's the old method in HibernatePatientDAO.java that only queried patients by identifier. This was removed in sometime after OpenMRS 1.7 I believe. The best option may be to put it back.

public List<Patient> getPatientsByIdentifier(String name, String identifier,
	List<PatientIdentifierType> identifierTypes, boolean matchIdentifierExactly) throws DAOException {
		
	Criteria criteria = sessionFactory.getCurrentSession().createCriteria(Patient.class);
	criteria = new PatientSearchCriteria(sessionFactory, criteria).prepareCriteria(name, identifier, identifierTypes,
	        matchIdentifierExactly);
	// restricting the search to the max search results value
	criteria.setFirstResult(0);
	criteria.setMaxResults(HibernatePersonDAO.getMaximumSearchResults());
		
	return criteria.list();
}

This is older code, so it may need updated to match newer query mechanisms of OpenMRS.

[kdaud]

@sjmckee I addressed the reviews respectively!

[kdaud]

@sjmckee hope you will get some time and take a look at the changes. Thanks!

[kdaud]

@sjmckee @dkayiwa one of the challenge have encountered is that when I changed to using the getPatientByIdentifier(), the EncounterServiceTest.getEncountersByPatientIdentifier_shouldNotGetVoidedEncounters() is not able to return the expected encounters.

Have tried out adding a new patient identifier in the xml file and used it for the test but things are not working out. Am I missing out something so obvious that am failing to figure out? Thanks!

[kdaud]

@dkayiwa @ibacher my shoulders are failing to fix an error occurring from switching Context.getPatientService().getPatients() to Context.getPatientService().getPatientsByIdentifier().

According to the log report I understand that the issue is coming from the EnounterServiceTest returning no encounter with the patient_identifier:4321 from the dataset. Have added a patient_identifier with value of 4321 but still the test is failing,

Am reaching out to you for support so that I can be in position to resolve the issue? Many thanks!