Implementation of `--activePeriod` feature. #152

bashir2 · 2021-04-03T08:42:54Z

Description of what I changed

This implements issue #128 by adding an --activePeriod feature. For details, see this comment.

E2E test

TESTED:
Ran:

$ java -cp batch/target/fhir-batch-etl-bundled-0.1.0-SNAPSHOT.jar org.openmrs.analytics.FhirEtl \
  --openmrsServerUrl=http://localhost:8099/openmrs --resourceList=Patient,Encounter,Observation \
  --targetParallelism=10 --numFileShards=3 --secondsToFlushFiles=1200 \
  --outputParquetPath=[PATH] --activePeriod=2020-11-10T00:00:00

Then compared the list of patientIds for whom the historical resources were fetched to uuids of the following MySQL query (this is not a perfect query but works for this case):

SELECT person.uuid, patient_id, COUNT(0) AS num_encounters
FROM person, encounter
WHERE person.person_id=encounter.patient_id AND encounter.voided=0
  AND encounter_datetime >= "2020-11-10"
GROUP BY patient_id ORDER BY patient_id

This list includes 171 patients (with the big test DB) and there are 17321 Observations associated to these patients which was confirmed by this MySQL query:

SELECT COUNT(0)
FROM obs, encounter
WHERE obs.encounter_id=encounter.encounter_id AND obs.voided=0 AND patient_id IN (
  SELECT patient_id FROM encounter WHERE encounter_datetime >= "2020-11-10");

Also ran this to check the two dates version of --activePeriod:

$ java -cp batch/target/fhir-batch-etl-bundled-0.1.0-SNAPSHOT.jar org.openmrs.analytics.FhirEtl \
  --openmrsServerUrl=http://localhost:8099/openmrs --resourceList=Patient,Encounter,Observation \
  --targetParallelism=10 --numFileShards=3 --secondsToFlushFiles=1200 \
  --outputParquetPath=[PATH] --activePeriod=2020-11-10T00:00:00_2020-11-20

Checklist: I completed these to help reviewers :)

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 am familiar with Google Style Guides for the language I have coded in.

No? Please take some time and review Java and Python style guides. Note, when in conflict, OpenMRS style guide overrules.
I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests) [More unit-tests as TODO.]

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and added all formatting changes to my commit.
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.
My pull request is based on the latest changes of the master branch.

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

codecov · 2021-04-03T08:59:53Z

Codecov Report

Merging #152 (85a7e79) into master (a65f482) will decrease coverage by 7.26%.
The diff coverage is 7.09%.

@@             Coverage Diff              @@
##             master     #152      +/-   ##
============================================
- Coverage     42.76%   35.50%   -7.27%     
- Complexity       97      102       +5     
============================================
  Files            21       24       +3     
  Lines           774      969     +195     
  Branches         67       87      +20     
============================================
+ Hits            331      344      +13     
- Misses          421      597     +176     
- Partials         22       28       +6

Impacted Files	Coverage Δ	Complexity Δ
...ava/org/openmrs/analytics/FetchPatientHistory.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...main/java/org/openmrs/analytics/FetchPatients.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../java/org/openmrs/analytics/FetchSearchPageFn.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...h/src/main/java/org/openmrs/analytics/FhirEtl.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...n/java/org/openmrs/analytics/DebeziumListener.java	`59.64% <ø> (ø)`	`5.00 <0.00> (ø)`
...ain/java/org/openmrs/analytics/FhirSearchUtil.java	`15.45% <2.38%> (-43.17%)`	`7.00 <4.00> (+1.00)`	⬇️
...ain/java/org/openmrs/analytics/FetchResources.java	`22.22% <22.22%> (ø)`	`2.00 <2.00> (?)`
...main/java/org/openmrs/analytics/JdbcFetchUtil.java	`59.03% <50.00%> (ø)`	`11.00 <0.00> (ø)`
...c/main/java/org/openmrs/analytics/OpenmrsUtil.java	`70.00% <50.00%> (-11.82%)`	`6.00 <2.00> (+2.00)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a65f482...85a7e79. Read the comment docs.

kimaina · 2021-04-06T11:55:00Z

batch/src/main/java/org/openmrs/analytics/FhirEtl.java

+				throw new IllegalArgumentException("--activePeriod is not supported in JDBC mode.");
+			}
+			Set<String> resourceSet = Sets.newHashSet(options.getResourceList().split(","));
+			if (resourceSet.contains("Patinet")) {


Done.
Please note that I changed the way Patient resources are handled in the --activePeriod mode. Hence, this check is now different.

kimaina · 2021-04-06T12:03:24Z

batch/src/main/java/org/openmrs/analytics/FetchResources.java

+
+	@VisibleForTesting
+	static String getSubjectPatientIdOrNull(Resource resource) {
+		String patinetId = null;


NIT: patientId

kimaina · 2021-04-06T12:18:16Z

batch/Dockerfile

@@ -24,7 +24,7 @@ ENV OPENMRS_PASSWORD="Admin123"
 ENV SINK_PATH=""
 ENV SINK_USERNAME=""
 ENV SINK_PASSWORD=""
-ENV SEARCH_LIST="Patient,Encounter,Observation"
+ENV RESOURCE_LIST="Patient,Encounter,Observation"


It would be nice to change docker-compose files e.g here https://github.com/GoogleCloudPlatform/openmrs-fhir-analytics/blob/353cb17d30ee0e74816a58d1d56eddfecb8c77a3/docker/docker-compose.yaml#L32

Done and thanks for catching this. Also updated the JDBC code-paths for consistency.

kimaina

This looks great, just a few NITs. I'll also comment on the results from yesterday's discussion!

bashir2

Given the tests we did last week, I changed this new feature to fetch Patient resources separately. The reason is that fetching all Patients may still time-out if there are millions of them. With the new implementation, only Patient resources are fetched that have an Observation/Encounter in the active period.

bashir2 · 2021-04-12T16:37:57Z

batch/Dockerfile

@@ -24,7 +24,7 @@ ENV OPENMRS_PASSWORD="Admin123"
 ENV SINK_PATH=""
 ENV SINK_USERNAME=""
 ENV SINK_PASSWORD=""
-ENV SEARCH_LIST="Patient,Encounter,Observation"
+ENV RESOURCE_LIST="Patient,Encounter,Observation"


Done and thanks for catching this. Also updated the JDBC code-paths for consistency.

bashir2 · 2021-04-12T16:43:14Z

batch/src/main/java/org/openmrs/analytics/FetchResources.java

+
+	@VisibleForTesting
+	static String getSubjectPatientIdOrNull(Resource resource) {
+		String patinetId = null;


bashir2 · 2021-04-12T18:25:10Z

batch/src/main/java/org/openmrs/analytics/FhirEtl.java

+				throw new IllegalArgumentException("--activePeriod is not supported in JDBC mode.");
+			}
+			Set<String> resourceSet = Sets.newHashSet(options.getResourceList().split(","));
+			if (resourceSet.contains("Patinet")) {


Done.
Please note that I changed the way Patient resources are handled in the --activePeriod mode. Hence, this check is now different.

kimaina

Thanks for addressing the suggestion! This looks great!

bashir2 added 2 commits April 3, 2021 03:44

Implementation of the --activePeriod feature.

c1811e7

fixed e2e and other refs to searchList

582b934

bashir2 changed the title ~~Date review~~ Implementation of --activePeriod feature. Apr 3, 2021

bashir2 requested a review from kimaina April 3, 2021 09:06

kimaina reviewed Apr 6, 2021

View reviewed changes

kimaina reviewed Apr 7, 2021

View reviewed changes

review comments and active patients

ff6b787

bashir2 commented Apr 12, 2021

View reviewed changes

bashir2 added 2 commits April 12, 2021 15:16

Merge branch 'master' into date_review

a60f82c

Merge branch 'master' into date_review

85a7e79

kimaina approved these changes Apr 14, 2021

View reviewed changes

bashir2 merged commit a540b5f into google:master Apr 14, 2021

bashir2 deleted the date_review branch April 22, 2021 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of `--activePeriod` feature. #152

Implementation of `--activePeriod` feature. #152

bashir2 commented Apr 3, 2021 •

edited

Loading

codecov bot commented Apr 3, 2021 •

edited

Loading

kimaina Apr 6, 2021

bashir2 Apr 12, 2021

kimaina Apr 6, 2021

bashir2 Apr 12, 2021

kimaina Apr 6, 2021

bashir2 Apr 12, 2021

kimaina left a comment

bashir2 left a comment

bashir2 Apr 12, 2021

bashir2 Apr 12, 2021

bashir2 Apr 12, 2021

kimaina left a comment

Implementation of --activePeriod feature. #152

Implementation of --activePeriod feature. #152

Conversation

bashir2 commented Apr 3, 2021 • edited Loading

Description of what I changed

E2E test

Checklist: I completed these to help reviewers :)

codecov bot commented Apr 3, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimaina left a comment

Choose a reason for hiding this comment

bashir2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimaina left a comment

Choose a reason for hiding this comment

Implementation of `--activePeriod` feature. #152

Implementation of `--activePeriod` feature. #152

bashir2 commented Apr 3, 2021 •

edited

Loading

codecov bot commented Apr 3, 2021 •

edited

Loading