Wip db id gen #75

dartartem · 2020-09-24T14:12:06Z

No description provided.

…d docker compose dependencies

cer · 2020-09-24T23:59:52Z

docker-compose-cdc-mariadb-binlog.yml

@@ -21,6 +21,7 @@ services:
      EVENTUATELOCAL_CDC_DB_USER_NAME: root
      EVENTUATELOCAL_CDC_DB_PASSWORD: rootpassword
      EVENTUATELOCAL_CDC_READER_NAME: MySqlReader
+      EVENTUATELOCAL_CDC_READER_ID: 1


See eventuate-foundation/eventuate-common#46 (comment)

cer · 2020-09-25T00:11:28Z

.circleci/config.yml

@@ -71,6 +71,21 @@ workflows:
      - build-and-test/build-and-test:
          name: build-and-test-postgres-wal-json
          script: ./scripts/build-and-test-postgres-wal-json.sh
+      - build-and-test/build-and-test:


This is an extremely expensive way to test a low-level feature. Every additional job adds minutes to the build:

Wait for an available VM

Create VM to run the job

Restore cache

compile lots of code

Run one test, e.g. io.eventuate.local.polling.PollingBinlogEntryReaderMessageTableTest

shut everything down

A more efficient approach would be a single job that perhaps runs first and starts all of the databases and runs the needed tests.

cer · 2020-09-25T00:18:01Z

...c-connector/src/main/java/io/eventuate/tram/cdc/connector/BinlogEntryToMessageConverter.java

+
+    if (!headers.containsKey("ID")) {
+      headers = new HashMap<>(headers);
+      headers.put("ID", idGenerator.genId(binlogEntry.getLongColumn("xid")).asString());


"xid" should not be a magic value. There should be a constant defined in eventuate-common

Agree, but usually we don't use constants for column names, and having only one can look a little inconsistent, no?
Or should we think about constants for all columns?

cer · 2020-09-25T00:19:05Z

mariadb-database-id/3.initialize-database.sql

@@ -0,0 +1,20 @@
+USE eventuate;
+
+CREATE TABLE new_message (


Isn't this in common?!

Yes, but the same approach is used for testing json schema. Probably is better to move such sqls to java code or gradle plugin, I did not think about it yet. What do you think?

No. My point is: why is the CDC building images that have already been built in eventuate-common?

cer · 2020-09-25T00:22:11Z

scripts/build-and-test-postgres-wal-database-id.sh

+export DATABASE=postgres
+export MODE=wal
+export TEST_MODULE=eventuate-local-java-cdc-connector-postgres-wal
+export TEST_CLASS=io.eventuate.local.postgres.wal.PostgresWalBinlogEntryReaderMessageTableTest


Why do these profiles postgres,PostgresWal need to be activated when running PostgresWalBinlogEntryReaderMessageTableTest which is specific to postgres,PostgresWal?

Chris, you probably ask why I added "postgres" to:

@ActiveProfiles("${SPRING_PROFILES_ACTIVE:PostgresWal}")

we need to use profiles like: "postgres, mssql" to activate properties that used to connect to those databases.

But PostgresWal is used only for postgres, and adding postgres as activated profile can't break something, but helps run that script from Idea. (I don't need to add it as env variable)

However postgres can be used with polling, so it does not make sense to have application-postgres-wal.properties, because we anyway need to have also application-postgres.properties with the same content.

No. My point is that @activeprofiles on PostgresWalBinlogEntryReaderMessageTableTest can activate the needed profiles `@ActiveProfiles("postgres,PostgresWal").

There is no need to specify SPRING_PROFILES_ACTIVE when executing PostgresWalBinlogEntryReaderMessageTableTest

Oh.. I see now, yes, I will remove it

cer · 2020-09-25T00:29:40Z

.../main/java/io/eventuate/tram/cdc/connector/pipeline/CdcTramPipelineFactoryConfiguration.java

@@ -25,6 +26,6 @@ public CdcPipelineFactory cdcPipelineFactory(DataProducerFactory dataProducerFac
                    publishingFilter,
                    new MessageWithDestinationPublishingStrategy(),
                    meterRegistry),
-            new BinlogEntryToMessageConverter());
+            readerId -> new BinlogEntryToMessageConverter(new DatabaseIdGenerator(readerId)));


I am confused by this.
readerId can be null, right?
DatabaseIdGenerator's constructor should/does verify that readerId is not null, right - since it needs one.

Chris,

tests uses both id generators, application and database.
Application id generator is used when reader id is not specified, that is why here null is allowed:
https://github.com/eventuate-foundation/eventuate-cdc/pull/75/files#diff-0dfbeeb39750b2d0117def81e57cbe28R20
Also it is allowed for database binlog clients.
But the cdc service requires reader id: https://github.com/eventuate-foundation/eventuate-cdc/pull/75/files#diff-fafb9b7680f553fb1b9f65d6bf4202c8R30

Regarding that line of code (29), reader id is Long (not long) because

BinlogEntryToEventConverterFactory (specified as lambda on line 29) is Function<Long, BinlogEntryToEventConverter>

and as we know Generics cannot be primitives.
Also I don't think that is critical to change BinlogEntryToEventConverterFactory to use explicit method definition.

I think you are miss understanding by comment.

readerId -> new BinlogEntryToMessageConverter(new DatabaseIdGenerator(readerId)) creates a BinlogEntryToMessageConverter

But what if readerId is null - i.e. a reader for a schema using old id generation. DatabaseIdGenerator expects a non-null readerId.

My logic was that when we migrate the cdc we should anyway specify a reader id.
But in case if messages were generated with application id, cdc does nothing, because ID header is present:

eventuate-cdc/eventuate-tram-cdc-connector/src/main/java/io/eventuate/tram/cdc/connector/BinlogEntryToMessageConverter.java

Lines 24 to 27 in 8a30146

if (!headers.containsKey("ID")) {

headers = new HashMap<>(headers);

headers.put("ID", idGenerator.genId(binlogEntry.getLongColumn("xid")).asString());

}

So you have added a new mandatory property for a reader: id which is in reality outboxId.
Not sure that makes sense if someone is using application generated IDs and they haven't thought about migrating to db generated IDs.

I think it makes migration more straightforward.
As I see a migration scenario:

User updates all libraries, including cdc.

Migrates schema.

Cdc forces him to specify unique id for reader (user should not think about it much, he should only know that id is unique and has 48 unsigned bits).

Everything works as before (application generated id)

At some point that user desides to use database generated id. He only uses outbox id for applications as he used for reader. Because other work already done during an update.

…timized database id and json testing. Removed redundant env variables.

…cdc.

…bcOperations.

dartartem added 3 commits September 18, 2020 11:19

eventuate-foundation#71: CircleCI needs to publish Docker image

29710c8

eventuate-foundation#71: CircleCI needs to publish Docker image. Fixe…

dcda477

…d docker compose dependencies

Supported database id generation.

8a30146

cer requested changes Sep 25, 2020

View reviewed changes

dartartem added 8 commits September 28, 2020 22:17

Renamed reader id to outbox id. Removed redundant database images, op…

17597ba

…timized database id and json testing. Removed redundant env variables.

Fixed db image versions.

31615b7

removed test docker images

9323084

Supported database id generation for eventuate local.

16f5c49

Events and messages with database flag published=true are ignored by …

c2256a6

…cdc.

Removed currentTimeInMillisecondsSql parameter from EventuateCommonJd…

1ca8660

…bcOperations.

Updated insert into message table in test.

051e2d9

Moved DOCKER_IMAGE_TAG to upper configuration level.

21762f5

cer merged commit e6d4ca0 into eventuate-foundation:wip-db-id-gen Oct 25, 2020

dartartem mentioned this pull request Nov 6, 2020

Wib db id gen (PLEASE CREATE BRANCH wip-db-id-gen) eventuate-tram/eventuate-tram-examples-customers-and-orders#72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wip db id gen #75

Wip db id gen #75

dartartem commented Sep 24, 2020

cer Sep 24, 2020

dartartem Sep 25, 2020

cer Sep 25, 2020

dartartem Sep 25, 2020

cer Sep 25, 2020

dartartem Sep 25, 2020

cer Sep 25, 2020

dartartem Sep 25, 2020

cer Sep 25, 2020 •

edited

Loading

cer Sep 25, 2020

dartartem Sep 25, 2020

cer Sep 25, 2020 •

edited

Loading

dartartem Sep 25, 2020

cer Sep 25, 2020

dartartem Sep 25, 2020

cer Sep 25, 2020 •

edited

Loading

dartartem Sep 25, 2020

cer Sep 25, 2020

dartartem Sep 25, 2020

	if (!headers.containsKey("ID")) {
	headers = new HashMap<>(headers);
	headers.put("ID", idGenerator.genId(binlogEntry.getLongColumn("xid")).asString());
	}

Wip db id gen #75

Wip db id gen #75

Conversation

dartartem commented Sep 24, 2020

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

cer Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cer Sep 25, 2020 • edited Loading

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

cer Sep 25, 2020 • edited Loading

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

cer Sep 25, 2020 •

edited

Loading

cer Sep 25, 2020 •

edited

Loading

cer Sep 25, 2020 •

edited

Loading