Platform 2.4 Release: To-do's and release discussions thread

ibacher · October 28, 2020, 1:44pm

I thought I was pretty transparent about my thought process above, but here it goes:

In your original log file, the error was reported to occur at DatabaseUpdater.java:188, which is just re-throwing an exception with some additional context information.
The original exception was that org\openmrs\liquibase\updates\liquibase-update-to-lates t-2.0.x.xml does not exist, but when I looked at DatabaseUpdater.java:244 (where the original exception was thrown), I saw it followed a call to liquibase.listUnrunChangeSets(). In order to be able to get the number of change sets that need to be run, Liquibase must be able to read the changelog file. So clearly the error was in our code (i.e., the file must actually exist and be readable or else we would’ve gotten an exception on line 232 instead of line 244).
I dug a bit into the source code for the listUnrunChangeSets() call and discovered that it gets the DatabaseChangeLog object by calling getDatabaseChangeLog(). Evidently, when calling getDatabaseChangeLog() the code was able to find the file, but when we tried to pass the file name to the parser on line 244, it couldn’t find it. (This is enough to see why I came up with that solution, but there were some more supporting points).
In your log, the path was written with usual Windows file separators (\), but inside the Liquibase class, it normalises the slashes to /. So what we were passing when it failed didn’t match what was being used when it succeeds.
When we create the Liquibase object, we pass it two ResourceAccessor objects (the classes that actual do the lookup), on that reads from a classloader, and one that reads from the file system.
When you’re running OpenMRS from the WAR file, the only way it’s going to find one of the built-in liquibase files is using the ClassLoaderFileOpener object. The sensible way to try to load a file from the classpath is ultimately to call cl.getResource(), but cl.getResource() only understands paths with /s, e.g., cl.getResource("org/openmrs/liquibase/updates/liquibase-update-to-latest-2.0.x.xml"), so if you passed it a path like org\openmrs\liquibase\updates\liquibase-update-to-lates t-2.0.x.xml, it would be unable to find the resource.

Basically, I’m a heavy user of my IDE’s “Go to Implementation” feature (I think it’s F3 in Eclipse, but I use Intellij on a Mac, so…). Hope that helps.

Incidentally, all my patch really does is replace a few lines with:

DatabaseChangeLog changeLog = liquibase.getDatabaseChangeLog();

The rest of the changes are just a bit of refactoring.

gcliff · October 28, 2020, 2:38pm

thanks @ibacher for the detailed explanation

wolf · October 28, 2020, 5:36pm

That is quite nifty, thanks for fixing this and sharing the details!

mogoodrich · October 28, 2020, 7:14pm

Okay, getting closer!

All the modules start now, but I’m getting some errors when trying to load resources. Not quite sure what is going on there, but right before they start there’s a stack overflow issue that possible could be hogging resources and breaking other things.

@ibacher @wolf do you know what deal is with this method? Looks like just a method that calls itself??

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/util/DatabaseUpdater.java#L145


	 * @param userInput nullable map from question to user answer. Used if a call to update(null) threw
	 *            an {@link InputRequiredException}
	 * @throws DatabaseUpdateException
	 * @throws InputRequiredException
	 */
	public static void executeChangelog(String changelog, Map<String, Object> userInput)
	        throws DatabaseUpdateException, InputRequiredException {
		
		log.debug("Executing changelog: " + changelog);
		
		executeChangelog(changelog, userInput);
	}
	
	/**
	 * Interface used for callbacks when updating the database. Implement this interface and pass it to
	 * {@link DatabaseUpdater#executeChangelog(String, ChangeSetExecutorCallback)}
	 */
	public interface ChangeSetExecutorCallback {
		
		/**
		 * This method is called after each changeset is executed.

Take care, Mark

ibacher · October 28, 2020, 7:34pm

The previous version at least didn’t have that infinite recursion, but I’ve never actually triggered that. Here’s a proposed fix.

mogoodrich · October 28, 2020, 8:18pm

Cool, thanks for the quick response @ibacher! I haven’t been following that class at all… I assume @wolf would be the best person to review your PR?

ibacher · October 28, 2020, 9:57pm

Yes, I would think so.

mogoodrich · October 29, 2020, 2:29pm

Cool, @wolf let me know when you’ve reviewed and we’ve merged and I can test again, thanks!

wolf · October 29, 2020, 6:07pm

Sure thing, the review is under way.

gcliff · November 9, 2020, 12:52pm

@wolf @ibacher any updates on the review?

, so that @mogoodrich can continue with the testing coz its currently a blocker for the release

grace · November 10, 2020, 1:09am

(FYI @gcliff, I had a chance to talk briefly w/ @ibacher today about test coverage for the FHIR API. Ian is confident that the existing automated tests cover the functionality that should work. However he would like to see those tests run against a distribution (e.g. we could test by running against the QA RefApp). Does this make sense or is more clarity needed for next steps with the FHIR API?

gcliff · November 10, 2020, 6:46am

thanks for the feedback @grace

the module runs well on a reffApp distribution

i think the QA team(@christine) can weigh in if this ready against the QA

gcliff · November 18, 2020, 8:50pm

hello @mogoodrich , the PR as been merged ,kindly test this out on the PIH EMR to see if the slight variations from just the standard install of 2.3 and then the upgrade to this version with these latest merged changes fixes the glitches …

Thanks

cc @dkayiwa

dkayiwa · November 18, 2020, 9:00pm

@gcliff i somehow feel that it may be easier for him and others to test if you first do another alpha release.

gcliff · November 18, 2020, 9:05pm

thanks ,let me work on it

gcliff · November 20, 2020, 5:38am

hello @mogoodrich , another alpha was released with the latest changes ,…Could you please test this out on the PIH EMR and give us some feedback. The artifacts were released to maven for core and platform.

Am currently doing a local test set-up with the new war file deployment of the latest changes

Thanks

cc @dkayiwa

gcliff · November 21, 2020, 6:16pm

hello @mogoodrich @mseaton i did a local test of the recently release 2.4.0-alpha.3 and it works fine

kindly give us some feedback on how your finding 2.4.0-alpha.3 on the PIH EMR .

I would also like to take this opportunity to invite other community member to test this out locally and share feedback here by doing a maven build of the this release by checking out into 2.4.0-alpha.3 branch. Download link

cc @dkayiwa @herbert24 @grace @ruhanga @k.joseph @jennifer @tendomart

tendomart · November 22, 2020, 1:08pm

@gcliff have you created a Test tracking list for 2.4.0-alpha.3 ,

or rather continue with the previous google sheet ?

tendomart · November 22, 2020, 1:52pm

Has anyone experienced this , firing up my 2.4.0 -alpha.3 instance lands on this page after login .

I cannot go to the SysAdmin page .

could it be a bug or there is a newest way of doing things i know not of.

gcliff · November 23, 2020, 5:02am

we can use the previous google tracking sheet