Add initial PostgreSQL implementation for get & put operations #5

G8XSU · 2023-01-13T01:57:56Z

TestStrategy

How?

PostgresIntegrationTest uses testcontainers to spin up docker containers for each test.
AbstractKVStoreIntegrationTest defines behavior that every impl of KVStore needs to follow. Different impl's can re-use the same abstract class to test their impl against it.
We will keep on adding tests to this as we add more impl details.

app/src/main/java/org/vss/impl/postgres/sql/v0_create_vss_db.sql

devrandom · 2023-01-18T11:28:55Z

app/build.gradle

+            generateSchemaSourceOnCompilation = true
+
+            generationTool {


we need to make a decision if we check in the generated Java code or not. personally, I'm a bit uncomfortable with generating code on the fly on prod machines, but let's see what other people think.

Discussed this offline. Prefer to check in the code but also provide a way for users to generate it at as an option to the build process.

app/src/main/java/org/vss/impl/postgres/PostgresBackendImpl.java

devrandom · 2023-01-18T12:02:55Z

app/src/main/java/org/vss/impl/postgres/PostgresBackendImpl.java

+
+    VssDbRecord globalVersionRecord = buildVssRecord(storeId,
+        KeyValue.newBuilder()
+            .setKey(GLOBAL_VERSION_KEY)


I think the global version should be stored in a separate table. otherwise, you have to make sure the caller doesn't try to use this key, and it seems messy.

The advantage is that it can be treated as any other keyvalue and easily handled with exactly same cases.

Other consideration is, if we want client to be able to override global_version in certain scenarios. My take is we should allow, end of the day its their storage and versioning, they can do whatever they want with it.

There is also case where client just wants to do a "get" on global_version.
For all purposes, it seems simpler to keep it as normal key-value, otherwise i will need to introduce additional code to support everything.

One case that we will have to handle separately from normal key-value is to not return global_version as part of ListKeyVersions api. (we have dedicated field in response for it)

app/src/main/java/org/vss/impl/postgres/PostgresBackendImpl.java

app/build.gradle

devrandom · 2023-01-18T14:57:01Z

also, would be good to have an integration test. not sure if it's better to have it against postgres (more precise model of a production environment) or against a lightweight in-memory SQL DB (faster to run the tests).

devrandom · 2023-01-19T10:36:03Z

app/src/main/java/org/vss/impl/postgres/PostgresBackendImpl.java

-            .setKey(GLOBAL_VERSION_KEY)
-            .setVersion(request.getGlobalVersion())
-            .build());
+    if (request.hasGlobalVersion()) {


this doesn't increment the global version when it's not specified in the request. we always want to increment it, right?

I guess we could have the specifications say that it doesn't get incremented if not present in the request, but we should be explicit about it, since the developer might not expect that

Based on how optimistic locking works, version needs to be provided by client in order to enact it.
So expectation is that we don't increment it and dont perform the check if developer doesn't supply it.

If global_version is not supplied, it means it is a non-global-version-check-required write.
We shouldn't be incrementing global_version in this case as client-side will not increment and has no way of knowing this without performing a sync.

Based on different application needs, some might not need a global_version check on every write, and this feature is meant to support those applications.

G8XSU · 2023-01-25T22:12:28Z

Added an integration test for AbstractKVStore and PostgresIntegration test uses it.
Can review it as part of this PR or we can separate it out as well in #5

devrandom · 2023-01-25T22:20:15Z

app/src/test/java/org/vss/impl/postgres/PostgresBackendImplIntegrationTest.java

+        postgreSQLContainer.getUsername(), postgreSQLContainer.getPassword());
+    DSLContext dslContext = DSL.using(conn, SQLDialect.POSTGRES);
+
+    this.kvStore = new PostgresBackendImpl(dslContext);


don't we need to close the previous DB connection, otherwise we may run out of file descriptors or such if we have many tests?

Each and every test spins up new db cluster/instance so db connection shouldn't really be a problem.
But I added a AfterEach block to destroy connection in any case.

For production, we are using connection pool and DSL.using(dataSource, dialect) where jooq/pool does the connection management for us.

jkczyz

Need to get through the tests still

jkczyz · 2023-02-02T16:36:22Z

app/build.gradle

+            generateSchemaSourceOnCompilation = true
+
+            generationTool {


Discussed this offline. Prefer to check in the code but also provide a way for users to generate it at as an option to the build process.

jkczyz · 2023-02-02T16:39:18Z

app/src/main/java/org/vss/KVStore.java

+
+  ListKeyVersionsResponse listKeyVersions(ListKeyVersionsRequest request);
+}
+


nit: no blanks at end of file here and throughout

jkczyz · 2023-02-02T16:42:39Z

app/src/main/java/org/vss/impl/postgres/sql/v0_create_vss_db.sql

@@ -0,0 +1,8 @@
+CREATE TABLE vss_db (
+    store_id character varying(120)  NOT NULL,


Is there a convention to have two spaces before NULL / NOT NULL?

idk, just a copy-paste side-effect from describe table in postgres i guess, will replace with single spaces.

jkczyz · 2023-02-02T16:47:12Z

app/src/main/java/org/vss/impl/postgres/sql/v0_create_vss_db.sql

+    store_id character varying(120)  NOT NULL,
+    key character varying(120)  NOT NULL,
+    value bytea  NULL,
+    version bigint  NOT NULL,


Was the choice of using a signed integer in the vss.proto based on the fact the database doesn't support unsigned types?

Mostly because many languages(kotlin/java/python) don't have good support for unsigned values.
So we avoid using them directly in interface if possible. (had this feedback from cashapp as well)

jkczyz · 2023-02-02T16:54:47Z

app/src/test/java/org/vss/impl/postgres/PostgresBackendImplIntegrationTest.java

+    dslContext.execute("CREATE TABLE vss_db ("
+        + "store_id character varying(120)  NOT NULL CHECK (store_id <> ''),"
+        + "key character varying(120)  NOT NULL,"
+        + "value bytea  NULL,"
+        + "version bigint  NOT NULL,"
+        + "PRIMARY KEY (store_id, key));");


Would it be possible to take this from the sql file?

Note that the sql file is present only for documentation purpose and git history.
Its not going to be directly used in db creation, will need to be done manually.
I wanted to avoid a filepath dependency in tests, let me know your thoughts.

Could creation be accomplished through a script / program that reads the schema?

jooq doesn't create table for us and we assume in production that db schema is already created.
In tests however we are creating a fresh instance of db for every test and need to create table/schema everytime.

jkczyz · 2023-02-02T16:56:15Z

app/src/test/java/org/vss/impl/postgres/PostgresBackendImplIntegrationTest.java

+
+  private void createTable(DSLContext dslContext) {
+    dslContext.execute("CREATE TABLE vss_db ("
+        + "store_id character varying(120)  NOT NULL CHECK (store_id <> ''),"


Why does this line differ from the sql file?

will fix this.

jkczyz · 2023-02-02T17:02:53Z

app/src/main/java/org/vss/impl/postgres/PostgresBackendImpl.java

+    } else {
+      keyValue = KeyValue.newBuilder()
+          .setKey(request.getKey()).build();
+    }


Should we return an error if there is no record for the key?

I think an empty response is preferable in this case.
ResourceNotFound or 404 might represent a myriad of issues and things going wrong. (url wrong etc.)
In this case, we consider it a perfectly valid request to get a key which does not exist or to check existence of a key.
I see no harm in mixing keys that exist with no data and keys which don't exist, to make clients life easier instead of throwing an exception in one case.

Note: This is only applicable if client has permission to storeId, if not then we would want to throw ResourceNotFound.

However, this is a controversial topic and there are reasons to go either way.

jkczyz · 2023-03-20T16:30:42Z

app/src/test/java/org/vss/impl/postgres/PostgresBackendImplIntegrationTest.java

+    dslContext.execute("CREATE TABLE vss_db ("
+        + "store_id character varying(120)  NOT NULL CHECK (store_id <> ''),"
+        + "key character varying(120)  NOT NULL,"
+        + "value bytea  NULL,"
+        + "version bigint  NOT NULL,"
+        + "PRIMARY KEY (store_id, key));");


Could creation be accomplished through a script / program that reads the schema?

jkczyz · 2023-03-20T16:57:16Z

app/src/test/java/org/vss/AbstractKVStoreIntegrationTest.java

+    KeyValue response = getObject("non_existent_key");
+
+    assertThat(response.getKey(), is("non_existent_key"));
+    assertTrue(response.getValue().isEmpty());


Also check that version is empty / zero?

jkczyz · 2023-03-20T20:02:31Z

app/src/main/java/org/vss/impl/postgres/PostgresBackendImpl.java

+      int[] batchResult = dsl.batch(batchQueries).execute();
+
+      for (int numOfRowsUpdated : batchResult) {
+        if (numOfRowsUpdated == 0) {
+          throw new ConflictException(
+              "Transaction could not be completed due to a possible conflict");
+        }
+      }
+    });


Could you test the case where one key failing causes the entire transaction to fail? (i.e., the successful key is not updated)

jkczyz

Looks like some of the comments for the first commit were resolved in the second commit. You'll want to make sure they are resolved in the right commit so that they are self-contained. Otherwise, looks good and sorry about the delay.

jkczyz · 2023-04-20T03:05:25Z

app/src/main/java/org/vss/impl/postgres/sql/v0_create_vss_db.sql

    PRIMARY KEY (store_id, key)
-);
-
+);


Add newline.

I earlier got comment about removing newlines/blanks at end of files,
bit confused, let me know if i misunderstood.

Sorry, please disregard. I thought that Github was showing the "No newline at end of file" symbol, but I guess I was mistaken.

Ok, i might have fixed it,
So just to clarify there "should" be a newline at EOF?

Addressed other comments as well.

Correct, it should contain a newline, just not an empty line.

G8XSU requested a review from jkczyz January 13, 2023 01:58

G8XSU changed the title ~~Add basic PostgreSQL implementation for get & put operations~~ Add initial PostgreSQL implementation for get & put operations Jan 13, 2023

G8XSU requested a review from devrandom January 17, 2023 22:36

devrandom reviewed Jan 17, 2023

View reviewed changes

app/src/main/java/org/vss/impl/postgres/sql/v0_create_vss_db.sql Outdated Show resolved Hide resolved

devrandom reviewed Jan 18, 2023

View reviewed changes

app/build.gradle Outdated Show resolved Hide resolved

devrandom reviewed Jan 19, 2023

View reviewed changes

G8XSU mentioned this pull request Jan 25, 2023

Add AbstractKVStoreIntegrationTest and PostgresImpl Test #6

Closed

devrandom reviewed Jan 25, 2023

View reviewed changes

G8XSU marked this pull request as ready for review January 26, 2023 01:38

jkczyz reviewed Feb 2, 2023

View reviewed changes

jkczyz reviewed Mar 20, 2023

View reviewed changes

G8XSU force-pushed the postgres-impl-get-put branch from 68a6729 to 344779e Compare April 12, 2023 01:41

G8XSU requested a review from jkczyz April 12, 2023 01:43

G8XSU mentioned this pull request Apr 19, 2023

Add Implementation for ListKeyVersions Api and AbstractKVStore tests for the same #8

Merged

jkczyz reviewed Apr 20, 2023

View reviewed changes

G8XSU added 2 commits April 20, 2023 17:02

Add basic PostgreSQL implementation for get & put operations

0ccda2f

Add AbstractKVStoreIntegrationTest and PostgresImpl Test

403b0c2

G8XSU force-pushed the postgres-impl-get-put branch from 344779e to 403b0c2 Compare April 21, 2023 00:03

jkczyz approved these changes Apr 21, 2023

View reviewed changes

G8XSU merged commit 5de859d into lightningdevkit:main Apr 21, 2023

G8XSU mentioned this pull request May 10, 2023

VSS Tracking Issue #9

Open

31 tasks


		ListKeyVersionsResponse listKeyVersions(ListKeyVersionsRequest request);
		}

		@@ -0,0 +1,8 @@
		CREATE TABLE vss_db (
		store_id character varying(120) NOT NULL,

Add initial PostgreSQL implementation for get & put operations #5

Add initial PostgreSQL implementation for get & put operations #5

Conversation

G8XSU commented Jan 13, 2023 • edited

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

devrandom commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G8XSU Jan 25, 2023 • edited

Choose a reason for hiding this comment

G8XSU commented Jan 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz 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

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

jkczyz 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

G8XSU commented Jan 13, 2023 •

edited

G8XSU Jan 25, 2023 •

edited