add ability to switch off/on creation of parquet dwh #1074

mozzy11 · 2024-05-29T10:01:54Z

Add ability to switch of creation of a parquet DWH in case of syncying betwen FHIR servers

Added a flag to the createParquetDwh to the controller to switch off/on creation of parquet DWH

E2E test

Adedd e2e tests for synching from a hapi fhir sever to another using the pipeline controller for both FULL and INCREMENTAL modes while swtching on/off creation of parquet DWH

TESTED:
Testes Locally syncying between FHIR server while the parquet DWH is switched off/on

Checklist: I completed these to help reviewers :)

I have read and will follow the review process.
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.
My IDE is configured to follow the Google code styles.

No? Unsure? -> configure your IDE.
I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)
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.
My pull request is based on the latest changes of the master branch.

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

codecov-commenter · 2024-05-29T10:08:42Z

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 51.99%. Comparing base (42c5e30) to head (e296d6c).

Files	Patch %	Lines
...java/com/google/fhir/analytics/DataProperties.java	40.00%	1 Missing and 2 partials ⚠️
...a/com/google/fhir/analytics/ConvertResourceFn.java	0.00%	0 Missing and 1 partial ⚠️
...a/com/google/fhir/analytics/FetchSearchPageFn.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1074      +/-   ##
============================================
- Coverage     52.03%   51.99%   -0.04%     
  Complexity      653      653              
============================================
  Files            89       89              
  Lines          5396     5402       +6     
  Branches        708      710       +2     
============================================
+ Hits           2808     2809       +1     
- Misses         2325     2328       +3     
- Partials        263      265       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mozzy11 · 2024-05-30T00:00:53Z

cc @bashir2

mozzy11 · 2024-07-08T18:15:58Z

cc @bashir2

bashir2 · 2024-07-09T16:01:20Z

cc @bashir2

Sorry @mozzy11 this fell off my radar (perhaps due to DevDays); thanks for the reminder. I'll provide some feedback by tomorrow.

bashir2

Thanks @mozzy11 for this change. I had a look and made some comments but in general I feel we need to think deeper about the implications of skipping Parquet file generation; I feel there are still scenarios not covered in your change (beyond what I have commented below) but need to think more about this.

bashir2 · 2024-07-10T23:54:35Z

pipelines/controller/config/application.yaml

@@ -73,6 +73,9 @@ fhirdata:
  # that directory too, such that files created by the pipelines are readable by
  # the Thrift Server, e.g., `setfacl -d -m o::rx dwh/`.
  dwhRootPrefix: "dwh/controller_DEV_DWH"
+  #Whether to create a  Parquet DWH or not.In case of syncying between a FHIR server to FHIR server , 


nit: Please also break long lines at 80 chars for YAML files (I know we have not followed it everywhere in this file but we should).

Suggested change

#Whether to create a Parquet DWH or not.In case of syncying between a FHIR server to FHIR server ,

# Whether to create a Parquet DWH or not. In case of syncing from a FHIR server to another, if Parquet files are not needed, their generation can be switched off by this flag.

bashir2 · 2024-07-10T23:58:03Z

pipelines/batch/src/main/java/com/google/fhir/analytics/FhirEtlOptions.java

@@ -260,4 +260,10 @@ public interface FhirEtlOptions extends BasePipelineOptions {
  String getSourceNDJsonFilePattern();

  void setSourceNDJsonFilePattern(String value);
+
+  @Description("Flag to switch off/on creation of a  parquet DWH")


Suggested change

@Description("Flag to switch off/on creation of a parquet DWH")

@Description("Flag to switch off/on creation of parquet files; can be turned off when syncing from a FHIR server to another.")

bashir2 · 2024-07-10T23:58:56Z

docker/config/application.yaml

@@ -25,6 +25,8 @@ fhirdata:
  # fhirServerUrl: "http://hapi-server:8080/fhir"
  dbConfig: "config/hapi-postgres-config_local.json"
  dwhRootPrefix: "/dwh/controller_DWH"
+  #Whether to create a Parquet DWH or not


You can probably drop this comment as we have a reference to pipelines/controller/config/application.yaml at the top for all comments.

bashir2 · 2024-07-11T04:36:14Z

docker/config_fhir_sink/application.yaml

@@ -0,0 +1,59 @@
+#


It seems that most of the content of this directory is a copy of config dir. Can you reuse those config files and only override the values that you need to change, e.g., with command-line arguments?

bashir2 · 2024-07-11T04:40:15Z

cloudbuild.yaml

+  id: 'Bring down controller and Spark containers for FHIR server to FHIR server sync'
+  args: [ '-f', './docker/compose-controller-spark-sql-single.yaml', 'down' ,'-v']
+
+# Resetting Sink FHIR server


It seems that these new tests are adding 15+ minutes to the e2e test run-time; I think changes in PR #947 had a similar effect too and we should try to reduce this. How about doing the sync test in one of the scenarios only and see how much it reduces the run-time? Maybe we can have only one scenario where sync is on and Parquet generation is off. Please also make sure that the incremental mode is tested in that scenario.

bashir2 · 2024-07-11T04:41:52Z

pipelines/batch/src/main/java/com/google/fhir/analytics/FetchSearchPageFn.java

@@ -200,7 +202,7 @@ public void setup() throws SQLException, ProfileException {
            oAuthClientSecret,
            fhirContext);
    fhirSearchUtil = new FhirSearchUtil(fetchUtil);
-    if (!Strings.isNullOrEmpty(parquetFile)) {
+    if (createParquetDwh) {


Do we have a sanity check if createParquetDwh is true but parquetFile is null or empty?

bashir2 · 2024-07-11T05:10:51Z

pipelines/controller/src/main/java/com/google/fhir/analytics/DataProperties.java

@@ -138,6 +143,7 @@ void validateProperties() {
      logger.info("Using FHIR-search mode since dbConfig is not set.");
    }
    Preconditions.checkState(!createHiveResourceTables || !thriftserverHiveConfig.isEmpty());
+    Preconditions.checkState(!createHiveResourceTables || createParquetDwh);


I think there are more config sanity check that needs to be done, e.g., when we are not generating Parquet files, generation of views should be disabled as well.

bashir2 · 2024-07-11T05:12:57Z

pipelines/controller/src/main/java/com/google/fhir/analytics/DataProperties.java

@@ -213,6 +219,8 @@ PipelineConfig createBatchOptions() {
        Instant.now().toString().replace(":", "-").replace("-", "_").replace(".", "_");
    options.setOutputParquetPath(dwhRootPrefix + TIMESTAMP_PREFIX + timestampSuffix);

+    options.setCreateParquetDwh(createParquetDwh);


Have you tested the incremental pipeline when this flag is turned off. In particular does the mergerPipelines here work fine? I think we need extra logic in PipelineManager to handle these edge cases.

add ability to switch off/on creation of parquet dwh

9ffab3a

mozzy11 force-pushed the isolate_fhir_sync branch from 2f13701 to 9ffab3a Compare May 29, 2024 22:46

mozzy11 added 2 commits May 31, 2024 06:55

Merge branch 'master' into isolate_fhir_sync

ef22c53

run e2e tests for parquet and fhir sink independently

f37bbeb

mozzy11 mentioned this pull request May 31, 2024

Turn sinkFhirServerUrl off by default but on in e2e #1075

Merged

7 tasks

mozzy11 added 4 commits June 3, 2024 18:17

Merge branch 'master' into isolate_fhir_sync

fbcf38c

Merge branch 'master' into isolate_fhir_sync

564d68a

Merge branch 'master' into isolate_fhir_sync

f2da0ea

Merge branch 'master' into isolate_fhir_sync

66ad736

Merge branch 'master' into isolate_fhir_sync

e296d6c

bashir2 reviewed Jul 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ability to switch off/on creation of parquet dwh #1074

add ability to switch off/on creation of parquet dwh #1074

mozzy11 commented May 29, 2024 •

edited

Loading

codecov-commenter commented May 29, 2024 •

edited

Loading

mozzy11 commented May 30, 2024

mozzy11 commented Jul 8, 2024

bashir2 commented Jul 9, 2024

bashir2 left a comment

bashir2 Jul 10, 2024

bashir2 Jul 10, 2024

bashir2 Jul 10, 2024

bashir2 Jul 11, 2024

bashir2 Jul 11, 2024

bashir2 Jul 11, 2024

bashir2 Jul 11, 2024

bashir2 Jul 11, 2024

	#Whether to create a Parquet DWH or not.In case of syncying between a FHIR server to FHIR server ,
	# Whether to create a Parquet DWH or not. In case of syncing from a FHIR server to another, if Parquet files are not needed, their generation can be switched off by this flag.

	@Description("Flag to switch off/on creation of a parquet DWH")
	@Description("Flag to switch off/on creation of parquet files; can be turned off when syncing from a FHIR server to another.")

add ability to switch off/on creation of parquet dwh #1074

Are you sure you want to change the base?

add ability to switch off/on creation of parquet dwh #1074

Conversation

mozzy11 commented May 29, 2024 • edited Loading

Add ability to switch of creation of a parquet DWH in case of syncying betwen FHIR servers

E2E test

Checklist: I completed these to help reviewers :)

codecov-commenter commented May 29, 2024 • edited Loading

Codecov Report

mozzy11 commented May 30, 2024

mozzy11 commented Jul 8, 2024

bashir2 commented Jul 9, 2024

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

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

mozzy11 commented May 29, 2024 •

edited

Loading

codecov-commenter commented May 29, 2024 •

edited

Loading