Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON configuration persistence 2 #39

Merged
merged 1 commit into from
Aug 12, 2020
Merged

Conversation

cgardens
Copy link
Contributor

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:

  1. The whole class is synchronized, to try to avoid any concurrency issues in the MVP.
  2. Added a couple exception to the persistence interface. The philosophy I used here was that
    1. 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.
    2. 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.

import java.util.Set;
import java.util.stream.Collectors;

public class JsonSchemaValidation {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a premature refactor here, but it get immediately used in a PR that i'm about to put up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proof: file: dataline-server/src/main/java/io/dataline/server/validation/IntegrationSchemaValidation.java in #42.

@@ -1,13 +1,28 @@
package io.dataline.config.persistence;

public enum PersistenceConfigType {
SOURCE_CONNECTION_CONFIGURATION,
// workspace
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't mean for this file to be in this PR. should be in this one: #41. not going to worry about moving it now. if there's anything you want to discuss in this file discuss in pr: #41, please.

@cgardens
Copy link
Contributor Author

@michel-tricot @sherifnada @sherifnada - this hard blocking progress on the API. going to merge. feel free to leave comments on this PR and i will incorporate them in future work.

@cgardens cgardens merged commit fba9e6e into master Aug 12, 2020
@cgardens cgardens deleted the charles/config_persistence2 branch August 12, 2020 15:54
Copy link
Contributor

@michel-tricot michel-tricot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants