JSON configuration persistence #17

cgardens · 2020-08-08T21:58:29Z

This PR creates an interface and implementation for how to persist configuration objects. Start with conduit-config-persistence/src/main/java/io/dataline/conduit/persistence/ConfigPersistence.java and conduit-config-persistence/src/main/java/io/dataline/conduit/persistence/ConfigPersistenceImpl.java.

The other changes in this PR are mostly just tweaks to the json schema.

cgardens · 2020-08-10T20:15:51Z

conduit-config-persistence/src/main/java/io/dataline/conduit/persistence/ConfigPersistence.java

+import java.util.Set;
+
+public interface ConfigPersistence {
+  <T> T getStandardConfig(


lmk if there's a better way to do the generics here. ultimately the jackson library needs to interact with a Class object and i didn't want to do any reflection.

sherifnada

So far so good!

sherifnada · 2020-08-10T21:17:21Z

conduit-config-persistence/src/main/java/io/dataline/conduit/persistence/ConfigPersistence.java

+
+public interface ConfigPersistence {
+  <T> T getConfig(
+      PersistenceConfigType persistenceConfigType, String configId, Class<T> clazz);


What is the purpose of PersistenceConfigType if we already provide clazz?

good question. clazz was a compromise implementation detail to get stuff to work. wish i could just get rid of that. if i drop the enum then the internals are going to have to do clazz.toString() and then run it through some switch statement. i think retaining the enum still makes what's going on clearer.

sherifnada · 2020-08-10T21:23:12Z

conduit-config-persistence/src/main/java/io/dataline/conduit/persistence/ConfigPersistence.java

+
+import java.util.Set;
+
+public interface ConfigPersistence {


Should all these methods throw so we can provide appropriate messages on the UI if needed?

what do you want them to throw? right now i am catching all I/O exceptions internally and trying to put reasonable error messages.

if we have a throw in the signature of the methods, then the server has to handle exceptions well. Unless the impl you're going for is that these methods would throw reasonable messages and the server autowraps any failures nicely in a 500/400 in the UI? (and therefore doesn't need to wrap each individual failure)

server autowraps any failures nicely in a 500/400

was not planning to do this.

can you give an example of what behavior you want? specifically what error you expect to have thrown and how the ui would interact with it?

if a config file is formatted incorrectly (fails validation), the UI should display a message saying for example: file is corrupt. Or if it is not found, should say file not found, please make sure your mount point is setup correctly. I'm imagining that since the server would be calling this persistence class, if the persistence class fails, we want to respond to the UI with a 400/500 with a human-readable message. I see two ways of doing this:

every method throws, and the server wraps that and throws based on position of the failure in control flow. This is higher friction since the UI server must try-catch persistence operations.

throw all un-fixable exceptions at the lowest level, and in the server whenever an exception is thrown, respond with a 500 whose message is exactly the failure message of the thrown exception

It seems like this impl is leaning towards no.2 above? this seems like a good choice since it's lowest friction.

can i punt on this until write some API endpoints and have a better feel for what will be ergonomic?

...-config-persistence/src/main/java/io/dataline/conduit/persistence/ConfigPersistenceImpl.java

jrhizor · 2020-08-10T22:15:34Z

...-config-persistence/src/main/java/io/dataline/conduit/persistence/ConfigPersistenceImpl.java

+    try {
+      objectMapper.writeValue(new File(getConfigPath(persistenceConfigType, configId)), config);
+    } catch (IOException e) {
+      throw new RuntimeException(e);


Shouldn't these methods throw an IOException instead of rethrowing as a runtime exception?

can i punt on this until write some API endpoints and have a better feel for what will be ergonomic?

Sounds good.

jrhizor · 2020-08-10T22:18:48Z

...-config-persistence/src/main/java/io/dataline/conduit/persistence/ConfigPersistenceImpl.java

+    return String.format("%s.json", id);
+  }
+
+  private ConfigSchema standardConfigTypeToConfigSchema(


What's the purpose of having two enums that are identical instead of just one?

ConfigSchema is mapping for name to file path. PersistenceConfigType is mean to be used as part of the interface but not tied to a file path.

Is there any expectation for these to diverge in the future? This sort of switch/mapping feels like a bit of an antipattern.

not sure yet. i'm already regretting custom json disk persistence due to lack of ability to query by anything other than id.

michel-tricot · 2020-08-11T00:29:55Z

conduit-config/build.gradle

@@ -31,4 +31,5 @@ dependencies {
 }

 jsonSchema2Pojo {
+    targetPackage = 'io.dataline.conduit.conduit_config'


Late to the party but it is weird to have conduit repeated twice. why not io.dataline.conduit.config

was just pattern matching other module names. the extra namespace can be dropped.

michel-tricot · 2020-08-11T00:30:28Z

settings.gradle

@@ -4,4 +4,4 @@ include 'conduit-api'
 include 'conduit-server'
 include 'conduit-commons'
 include 'conduit-config'
-
+include 'conduit-config-persistence'


Why did you decide to create an other module and not put it in conduit-config?

so that the json to pojo codegen can be isolated in a single package and then universally consumed externally.

In response to feedback on this PR #17 and after fiddling around with what implementing some endpoints looks like, I've made two updates to the configuration persistence: * The whole class is synchronized, to try to avoid any concurrency issues in the MVP. * Added a couple exception to the persistence interface. The philosophy I used here was that * we expose named exceptions that are actionable to the user. e.g. config not found, or you are trying to insert or access a config that does not pass validation. * we do NOT expose named exceptions related to the internal of the database (e.g. data corruption or we can't access the disk). we let these get handled as runtime exceptions.

cgardens added 9 commits August 8, 2020 14:57

wip

0fde438

whatever

0736bdc

garbage

f86a8b9

not a swear word

5732f58

blah

ac4543c

learn about validor and json to pojo

e34ae7c

persistence first draft

c76c28b

apply styles

808fbfb

remove example json objects

f3be34a

cgardens changed the title ~~rw for json~~ JSON configuration persistence Aug 10, 2020

cgardens marked this pull request as ready for review August 10, 2020 20:09

cgardens requested review from michel-tricot, jrhizor and sherifnada August 10, 2020 20:09

cgardens commented Aug 10, 2020

View reviewed changes

remove standard namespace from iface

1f77a02

sherifnada reviewed Aug 10, 2020

View reviewed changes

jrhizor reviewed Aug 10, 2020

View reviewed changes

apply style

38ad881

cgardens merged commit 4319ba4 into master Aug 10, 2020

cgardens deleted the charles/rw_json branch August 10, 2020 22:32

michel-tricot reviewed Aug 11, 2020

View reviewed changes

cgardens mentioned this pull request Aug 11, 2020

JSON configuration persistence 2 #39

Merged

sbjorn mentioned this pull request Aug 29, 2022

🎉 New Source: Adjust #16051

Merged

18 tasks

danilo-dti mentioned this pull request Nov 12, 2022

🎉 New Source: Microsoft Dataverse [python cdk] #18646

Merged

37 tasks

c-p-b pushed a commit that referenced this pull request Feb 19, 2023

delete docusaurus utils (#17)

f422a2d

avirajsingh7 mentioned this pull request Aug 3, 2023

🎉 New Source : Avni #28141

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON configuration persistence #17

JSON configuration persistence #17

cgardens commented Aug 8, 2020 •

edited

Loading

cgardens Aug 10, 2020

sherifnada left a comment

sherifnada Aug 10, 2020

cgardens Aug 10, 2020

sherifnada Aug 10, 2020

cgardens Aug 10, 2020

sherifnada Aug 10, 2020 •

edited

Loading

cgardens Aug 10, 2020

sherifnada Aug 10, 2020 •

edited

Loading

cgardens Aug 10, 2020

jrhizor Aug 10, 2020

cgardens Aug 10, 2020

jrhizor Aug 10, 2020

jrhizor Aug 10, 2020

cgardens Aug 10, 2020

jrhizor Aug 10, 2020

cgardens Aug 11, 2020

michel-tricot Aug 11, 2020

cgardens Aug 11, 2020

michel-tricot Aug 11, 2020

cgardens Aug 11, 2020

JSON configuration persistence #17

JSON configuration persistence #17

Conversation

cgardens commented Aug 8, 2020 • edited Loading

Choose a reason for hiding this comment

sherifnada 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

sherifnada Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherifnada Aug 10, 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

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

cgardens commented Aug 8, 2020 •

edited

Loading

sherifnada Aug 10, 2020 •

edited

Loading

sherifnada Aug 10, 2020 •

edited

Loading