Move debezium specific settings to the json config file #57

gitcliff · 2020-11-10T07:18:46Z

link:#51

moved the the debezium settings to json config file
added Debezium configurations class to the model

e2e test
TESTED:

mvn compile exec:java -pl streaming-binlog \
   -Dexec.args="--databaseHostName=localhost \
   --databasePort=3308 --databaseUser=root --databasePassword=Admin123\
   --databaseName=mysql --databaseSchema=openmrs --databaseServerId=223344 \
   --openmrsUserName=admin --openmrsPassword=Admin123 \
   --openmrsServerUrl=http://localhost:8081/openmrs \
   --fhirSinkPath=http://localhost:8888/fhir \
   --sinkUserName=hapi --sinkPassword=hapi123 \
   --fileParquetPath=/tmp/
```"

google-cla · 2020-11-10T07:18:51Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

utils/general_config.json

mozzy11 · 2020-11-10T13:20:46Z

@gitcliff can you also complete the Google's Contributor License Agreement (CLA) ??

utils/general_config.json

streaming-binlog/src/main/java/org/openmrs/analytics/DebeziumListener.java

bashir2

@gitcliff here is a suggestion: Please either create a new PR or copy the template here at the top of your PR description and make sure you have followed all steps. In particular, please add a TESTED field describing how you have e2e tested your change. I don't think your changes actually work.

streaming-binlog/src/main/java/org/openmrs/analytics/DebeziumListener.java

gitcliff · 2021-01-11T05:06:16Z

@bashir2 @kimaina @mozzy11 kindly review

streaming-binlog/src/main/java/org/openmrs/analytics/model/DebeziumConfiguration.java

kimaina

Thank you @gitcliff for working on this? LGTM, just one question on how we can add the flexibility of adding more DBZ configs in the json without the need of extending and recompiling the config model

gitcliff · 2021-01-12T08:59:47Z

@kimaina thanks for the review ,,
not yet quite sure on how to implement this ,though still googling around to find something
cc @bashir2

bashir2

Thanks @gitcliff for working on this. When you address the comments below, please also rerun your integration test. Also please update your TESTED field and describe how you make sure that new events are properly processed. You can either manually trigger Debezium events by adding new resources, e.g., patients/observations OR you can replay some past events.

Please also update this README section based on your changes.

bashir2 · 2021-01-20T02:29:52Z

streaming-binlog/src/main/java/org/openmrs/analytics/DebeziumListener.java

+	private String getDebeziumConfig() throws IOException {
+		GeneralConfiguration generalConfiguration = loadDebeziumConfig();
+		DebeziumConfiguration debeziumConfigs = generalConfiguration.getDebeziumConfigurations();
+		return "debezium-mysql:" + debeziumConfigs.getDatabaseHostName() + "?" + "databaseHostname="


Please remove the params in DebeziumArgs that are not used anymore; for example, databaseHostName, databasePort, etc.

bashir2 · 2021-01-20T02:50:07Z

streaming-binlog/src/main/java/org/openmrs/analytics/DebeziumListener.java

+	@VisibleForTesting
+	GeneralConfiguration loadDebeziumConfig() throws IOException {
+		Gson gson = new Gson();
+		String pathToFile = this.getClass().getClassLoader().getResource("debezium_config.json").getPath();


Please make a command line argument to pass the JSON config filename and set the default value properly. Also move the config from resources directory to utils/ similar to other configs.

If you merge the two JSON configs (which I prefer because you have already merged them in GeneralConfiguration) then that is already done in DebeziumArgs.fhirDebeziumEventConfigPath at the bottom of this file; we can rename that param to be more generic, e.g., fhirDebeziumConfigPath.

streaming-binlog/src/main/resources/debezium_config.json

bashir2 · 2021-01-20T03:08:19Z

streaming-binlog/src/main/java/org/openmrs/analytics/model/GeneralConfiguration.java

-	// TODO add Debezium config to the model + json
+	@Getter
+	@Setter
+	private DebeziumConfiguration debeziumConfigurations;


How about not having a separate JSON config file and merge the content of debezium_config.json into dbz_event_to_fhir_config.json?

@bashir2 thatnks for the review and feedback
The idea of merging the debezium config into dbz_event_to_fhir_config.json sounds good but it brings compilation incompatibilities when your fetching event configurations for debezium connection from the GeneralConfiguration with something like this EventConfiguration eventConfigs = generalConfiguration.getEventConfigurations(); .
Thats why i had separated them

Am seeing this is because the the event configurations in the general config class is a LinkedHashMap

@bashir2 kindly requesting for your feedback

bashir2 · 2021-01-20T03:11:37Z

streaming-binlog/src/main/resources/debezium_config.json

+      "databaseOffsetStorage" : "data/offset.dat",
+      "databaseHistory" : "data/dbhistory.dat",
+      "snapshotMode" : "initial",
+      "fhirDebeziumEventConfigPath" : "./utils/dbz_event_to_fhir_config.json",


Where is this path used? And why is it not in DebeziumConfiguration?

going to remove it

bashir2 · 2021-01-20T03:23:43Z

streaming-binlog/src/main/java/org/openmrs/analytics/DebeziumListener.java

+	}
+
+	@VisibleForTesting
+	GeneralConfiguration loadDebeziumConfig() throws IOException {


Does this actually load a full GeneralConfiguration? It seems that this method only fills the debeziumConfiguration part right? (I understand it is enough for the use-case here but it is a little bit confusing.)

I have made some suggestions below to merge the two JSON files. Once you do that, please also move this function to be a static factory method inside GeneralConfiguration and use it everywhere that we need a GeneralConfiguration instance.

streaming-binlog/src/main/java/org/openmrs/analytics/model/DebeziumConfiguration.java

bashir2 · 2021-01-20T04:59:59Z

BTW, when you address the comments, please push a separate commit (and not amend the first one); that will make the review process easier.

mozzy11 · 2021-01-20T11:38:33Z

oh ya , @gitcliff , even for the Updated PR conventions , we no longer squash commits after a review comment

bashir2 · 2021-01-27T05:44:46Z

@gitcliff please ping me when you are done with the changes we discussed today, in particular when Travis passes; thanks.

gitcliff · 2021-02-01T11:44:58Z

created a new pr here https://github.com/GoogleCloudPlatform/openmrs-fhir-analytics/pull/103

gitcliff changed the title ~~Move:Move debezium specific settings to the json config file~~ Move debezium specific settings to the json config file Nov 10, 2020

gitcliff mentioned this pull request Nov 10, 2020

Move debezium specific settings to the json config file #51

Closed

mozzy11 reviewed Nov 10, 2020

View reviewed changes

utils/general_config.json Outdated Show resolved Hide resolved

mozzy11 reviewed Nov 25, 2020

View reviewed changes

utils/general_config.json Outdated Show resolved Hide resolved

gitcliff force-pushed the Move branch from 18fe237 to 5d4830f Compare November 30, 2020 09:44

mozzy11 reviewed Nov 30, 2020

View reviewed changes

utils/general_config.json Outdated Show resolved Hide resolved

streaming-binlog/src/main/java/org/openmrs/analytics/DebeziumListener.java Outdated Show resolved Hide resolved

bashir2 reviewed Dec 2, 2020

View reviewed changes

streaming-binlog/src/main/java/org/openmrs/analytics/DebeziumListener.java Outdated Show resolved Hide resolved

streaming-binlog/src/main/java/org/openmrs/analytics/DebeziumListener.java Outdated Show resolved Hide resolved

gitcliff force-pushed the Move branch from 5d4830f to 4f3a75e Compare December 9, 2020 05:41

gitcliff force-pushed the Move branch from 2472766 to 9836c20 Compare January 10, 2021 14:53

kimaina reviewed Jan 12, 2021

View reviewed changes

streaming-binlog/src/main/java/org/openmrs/analytics/model/DebeziumConfiguration.java Outdated Show resolved Hide resolved

kimaina reviewed Jan 12, 2021

View reviewed changes

gitcliff self-assigned this Jan 12, 2021

bashir2 reviewed Jan 20, 2021

View reviewed changes

gitcliff force-pushed the Move branch from 9836c20 to 170f177 Compare January 21, 2021 18:30

gitcliff mentioned this pull request Jan 26, 2021

Add the flexibility of adding more debezium configs in the json #97

Open

gitcliff force-pushed the Move branch from 438e9f1 to 83759f6 Compare January 31, 2021 07:46

gitcliff closed this Feb 1, 2021

gitcliff force-pushed the Move branch from 83759f6 to 40a7b76 Compare February 1, 2021 05:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move debezium specific settings to the json config file #57

Move debezium specific settings to the json config file #57

gitcliff commented Nov 10, 2020 •

edited

google-cla bot commented Nov 10, 2020

mozzy11 commented Nov 10, 2020

bashir2 left a comment

gitcliff commented Jan 11, 2021

kimaina left a comment

gitcliff commented Jan 12, 2021

bashir2 left a comment

bashir2 Jan 20, 2021

bashir2 Jan 20, 2021

bashir2 Jan 20, 2021

gitcliff Jan 20, 2021 •

edited

gitcliff Jan 26, 2021

bashir2 Jan 20, 2021

gitcliff Jan 26, 2021

bashir2 Jan 20, 2021

bashir2 commented Jan 20, 2021

mozzy11 commented Jan 20, 2021

bashir2 commented Jan 27, 2021

gitcliff commented Feb 1, 2021

Move debezium specific settings to the json config file #57

Move debezium specific settings to the json config file #57

Conversation

gitcliff commented Nov 10, 2020 • edited

google-cla bot commented Nov 10, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

mozzy11 commented Nov 10, 2020

bashir2 left a comment

Choose a reason for hiding this comment

gitcliff commented Jan 11, 2021

kimaina left a comment

Choose a reason for hiding this comment

gitcliff commented Jan 12, 2021

bashir2 left a comment

Choose a reason for hiding this comment

bashir2 Jan 20, 2021

Choose a reason for hiding this comment

bashir2 Jan 20, 2021

Choose a reason for hiding this comment

bashir2 Jan 20, 2021

Choose a reason for hiding this comment

gitcliff Jan 20, 2021 • edited

Choose a reason for hiding this comment

gitcliff Jan 26, 2021

Choose a reason for hiding this comment

bashir2 Jan 20, 2021

Choose a reason for hiding this comment

gitcliff Jan 26, 2021

Choose a reason for hiding this comment

bashir2 Jan 20, 2021

Choose a reason for hiding this comment

bashir2 commented Jan 20, 2021

mozzy11 commented Jan 20, 2021

bashir2 commented Jan 27, 2021

gitcliff commented Feb 1, 2021

gitcliff commented Nov 10, 2020 •

edited

gitcliff Jan 20, 2021 •

edited