Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose the mass ingestion mode for the dockerized hapi server #693

Merged
merged 15 commits into from
Jun 14, 2024

Conversation

barabo
Copy link
Contributor

@barabo barabo commented Jun 12, 2024

This change is to expose the mass_ingestion mode from the dockerized jpa server.

@barabo
Copy link
Contributor Author

barabo commented Jun 13, 2024

@jamesagnew - I think this will do the trick, but since I'm a first-time contributor, the GHA runners won't start without approval. No rush, just wanted to get this into your inbox.

@KevinDougan
Copy link
Collaborator

@barabo This didn't compile, is it working for you locally? If so, did you forget to check-in some code?

#24 12.75 [INFO] -------------------------------------------------------------
#24 12.75 [ERROR] COMPILATION ERROR : 
#24 12.75 [INFO] -------------------------------------------------------------
#24 12.75 [ERROR] /tmp/hapi-fhir-jpaserver-starter/src/main/java/ca/uhn/fhir/jpa/starter/common/FhirServerConfigCommon.java:[103,20] error: cannot find symbol
#24 12.75   symbol:   method setEnableMassIngestionMode(Boolean)
#24 12.75   location: variable jpaStorageSettings of type JpaStorageSettings
#24 12.75 [INFO] 1 error

@jamesagnew
Copy link
Contributor

@barabo it looks like the method should be called setMassIngestionMode, not setEnableMassIngestionMode

@barabo
Copy link
Contributor Author

barabo commented Jun 13, 2024

@barabo it looks like the method should be called setMassIngestionMode, not setEnableMassIngestionMode

Oh, I see! It's referencing this.

@barabo
Copy link
Contributor Author

barabo commented Jun 13, 2024

@barabo This didn't compile, is it working for you locally? If so, did you forget to check-in some code?

#24 12.75 [INFO] -------------------------------------------------------------
#24 12.75 [ERROR] COMPILATION ERROR : 
#24 12.75 [INFO] -------------------------------------------------------------
#24 12.75 [ERROR] /tmp/hapi-fhir-jpaserver-starter/src/main/java/ca/uhn/fhir/jpa/starter/common/FhirServerConfigCommon.java:[103,20] error: cannot find symbol
#24 12.75   symbol:   method setEnableMassIngestionMode(Boolean)
#24 12.75   location: variable jpaStorageSettings of type JpaStorageSettings
#24 12.75 [INFO] 1 error

Sorry about that! I've been making edits in the GH ui without compiling (or understanding what was being referenced - long-time user, first-time contributor, here).

@barabo barabo closed this Jun 13, 2024
@barabo barabo reopened this Jun 13, 2024
Copy link
Contributor Author

@barabo barabo left a comment

Choose a reason for hiding this comment

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

oops, you can ignore this comment.

Copy link
Contributor Author

@barabo barabo left a comment

Choose a reason for hiding this comment

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

PTAL - I think it's finally working!

@KevinDougan KevinDougan mentioned this pull request Jun 14, 2024
@KevinDougan
Copy link
Collaborator

@barabo

I made some suggestions so that this new property would follow the same pattern as the other similar properties (see bulk_import_enabled).

@barabo
Copy link
Contributor Author

barabo commented Jun 14, 2024

@barabo

I made some suggestions so that this new property would follow the same pattern as the other similar properties (see bulk_import_enabled).

Does bulk_import_enabled set mass_ingestion_mode? If so, that might obviate this PR.

@KevinDougan
Copy link
Collaborator

@barabo
I made some suggestions so that this new property would follow the same pattern as the other similar properties (see bulk_import_enabled).

Does bulk_import_enabled set mass_ingestion_mode? If so, that might obviate this PR.

No, they are unrelated config settings.

I referred you to look at bulk_import_enabled simply to see how that particular property was being handled, and to see what the typical naming convention looks like for other Boolean properties - they have the enabled part at the END of the name, not the beginning. Therefore, I changed your PR to follow that same name pattern convention.

If you agree, please apply these suggestions and then refresh your local copy and do a full build to ensure that everything compiles and works as expected. Once you confirm everything on your local machine, ping me here and I will run the Workflows in the PR.

@barabo
Copy link
Contributor Author

barabo commented Jun 14, 2024

If you agree, please apply these suggestions and then refresh your local copy and do a full build to ensure that everything compiles and works as expected. Once you confirm everything on your local machine, ping me here and I will run the Workflows in the PR.

Ah, OK. I don't see the suggestions, but maybe I'm looking in the wrong place. Can you confirm that you submitted the review?

Copy link
Collaborator

@KevinDougan KevinDougan left a comment

Choose a reason for hiding this comment

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

Here are my suggested changes.

src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java Outdated Show resolved Hide resolved
src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java Outdated Show resolved Hide resolved
src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java Outdated Show resolved Hide resolved
src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java Outdated Show resolved Hide resolved
src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java Outdated Show resolved Hide resolved
src/main/resources/application.yaml Outdated Show resolved Hide resolved
barabo and others added 7 commits June 14, 2024 11:17
Co-authored-by: Kevin Dougan SmileCDR <72025369+KevinDougan-SmileCDR@users.noreply.github.com>
Co-authored-by: Kevin Dougan SmileCDR <72025369+KevinDougan-SmileCDR@users.noreply.github.com>
Co-authored-by: Kevin Dougan SmileCDR <72025369+KevinDougan-SmileCDR@users.noreply.github.com>
Co-authored-by: Kevin Dougan SmileCDR <72025369+KevinDougan-SmileCDR@users.noreply.github.com>
Co-authored-by: Kevin Dougan SmileCDR <72025369+KevinDougan-SmileCDR@users.noreply.github.com>
Co-authored-by: Kevin Dougan SmileCDR <72025369+KevinDougan-SmileCDR@users.noreply.github.com>
Co-authored-by: Kevin Dougan SmileCDR <72025369+KevinDougan-SmileCDR@users.noreply.github.com>
Copy link
Contributor Author

@barabo barabo left a comment

Choose a reason for hiding this comment

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

I think the suggestions are all applied and everything should work, but when I try to run in my environment I get an error from the build enforcer, which seems to be unrelated to the changes.

@barabo ➜ /workspaces/hapi-fhir-jpaserver-starter (master) $  mvn -B package --file pom.xml -Dmaven.test.skip=true
[INFO] Scanning for projects...
[WARNING] The project ca.uhn.hapi.fhir:hapi-fhir-jpaserver-starter:war:7.2.0 uses prerequisites which is only intended for maven-plugin projects but not for non maven-plugin projects. For such purposes you should use the maven-enforcer-plugin. See https://maven.apache.org/enforcer/enforcer-rules/requireMavenVersion.html
[INFO] 
[INFO] ------------< ca.uhn.hapi.fhir:hapi-fhir-jpaserver-starter >------------
[INFO] Building HAPI FHIR JPA Server - Starter Project 7.2.0
[INFO]   from pom.xml
[INFO] --------------------------------[ war ]---------------------------------
[INFO] 
[INFO] --- enforcer:3.3.0:enforce (enforce-maven) @ hapi-fhir-jpaserver-starter ---
[INFO] Rule 0: org.apache.maven.enforcer.rules.version.RequireMavenVersion passed
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  3.292 s
[INFO] Finished at: 2024-06-14T17:44:18Z
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.3.0:enforce (enforce-maven) on project hapi-fhir-jpaserver-starter: 
[ERROR] Rule 1: org.apache.maven.enforcer.rules.version.RequireJavaVersion failed with message:
[ERROR] HAPI FHIR is targeting JDK 11 for published binaries,
[ERROR]                                                                                 but we require JDK 17 to build and test this library.
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

I tried updating the jdk on my system to 17, but that didn't seem to change it. I noticed that the travis file was still specifying jdk11, but changing that to 17 didn't help either.

I'm afraid it's been too long since I've worked in a java environment, and I'm not sure how to make maven happy.

@barabo barabo requested a review from KevinDougan June 14, 2024 17:47
Copy link
Collaborator

@KevinDougan KevinDougan left a comment

Choose a reason for hiding this comment

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

I checked out your branch, built it locally and then ran mvn spring-boot:run using both true and false for that new config setting and everything seemed to work fine.

@KevinDougan KevinDougan merged commit cfc4d70 into hapifhir:master Jun 14, 2024
3 checks passed
@barabo
Copy link
Contributor Author

barabo commented Jun 14, 2024

Thank you @KevinDougan-SmileCDR and @jamesagnew! I can't wait to try it out!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants