-
Notifications
You must be signed in to change notification settings - Fork 13
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
Allow replacing the DocumentDBSchemaService #477
base: develop
Are you sure you want to change the base?
Allow replacing the DocumentDBSchemaService #477
Conversation
* [AD-1014] Developer Guide. * Commit Code Coverage Badge * [AD-1014] Updates to use existing GETTING_STARTED.md and added schema-caching.md * Commit Code Coverage Badge Co-authored-by: birschick-bq <[email protected]>
* Bump JDBC version from 1.4.1 to 1.4.2 * Commit Code Coverage Badge Co-authored-by: affonsoBQ <[email protected]>
* Bump commons-text from 1.9 to 1.10.0 Bumps commons-text from 1.9 to 1.10.0. --- updated-dependencies: - dependency-name: org.apache.commons:commons-text dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump com.github.spotbugs from 5.0.10 to 5.0.13 Bumps com.github.spotbugs from 5.0.10 to 5.0.13. --- updated-dependencies: - dependency-name: com.github.spotbugs dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Bump mockito-core from 4.8.0 to 4.8.1 Bumps [mockito-core](https://github.com/mockito/mockito) from 4.8.0 to 4.8.1. - [Release notes](https://github.com/mockito/mockito/releases) - [Commits](mockito/mockito@v4.8.0...v4.8.1) --- updated-dependencies: - dependency-name: org.mockito:mockito-core dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Commit Code Coverage Badge * Bump sslcontext-kickstart from 7.4.7 to 7.4.8 Bumps [sslcontext-kickstart](https://github.com/Hakky54/sslcontext-kickstart) from 7.4.7 to 7.4.8. - [Release notes](https://github.com/Hakky54/sslcontext-kickstart/releases) - [Changelog](https://github.com/Hakky54/sslcontext-kickstart/blob/master/CHANGELOG.md) - [Commits](Hakky54/sslcontext-kickstart@v7.4.7...v7.4.8) --- updated-dependencies: - dependency-name: io.github.hakky54:sslcontext-kickstart dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Bump checkstyle from 10.3.4 to 10.4 Bumps [checkstyle](https://github.com/checkstyle/checkstyle) from 10.3.4 to 10.4. - [Release notes](https://github.com/checkstyle/checkstyle/releases) - [Commits](checkstyle/checkstyle@checkstyle-10.3.4...checkstyle-10.4) --- updated-dependencies: - dependency-name: com.puppycrawl.tools:checkstyle dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Revert "Bump checkstyle from 10.3.4 to 10.4" This reverts commit b4b048d. * Adding mongo driver information * fixing build.gradle * adding missing class * Revert "adding missing class" This reverts commit 5a53d21. * adding missing class * adding license * fixing check style * adding final test fixture * Commit Code Coverage Badge * Commit Code Coverage Badge * Commit Code Coverage Badge * [AD-1032] Driver / Application information for CloudWatch. Simplify application and version handling. * Commit Code Coverage Badge * [AD-1032] Refactored creating the MongoClient for cleaner use. * [AD-1032] Fixed style issues. * [AD-1032] Fixed style issues aws#2. * [AD-1032] Fixed style issues aws#3. * Commit Code Coverage Badge Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Bruce Irschick <[email protected]> Co-authored-by: birschick-bq <[email protected]> Co-authored-by: affonsoBQ <[email protected]>
* [AD-1017] Implement single-instance SSH tunnel. * Handle late-bound invocation (DbVisualizer) * Ensure support for multiple hash algorithms. * Properly embed new SSH tunnel component. * [AD-1017] Attempt to debug multi-process test. * [AD-1017] Attempt to debug multi-process test. * [AD-1017] Attempt to debug multi-process test - aws#3. * Commit Code Coverage Badge * [AD-1017] Attempt to find correct location for test source file. * [AD-1017] Attempt to find correct location for test source file - aws#2. * [AD-1017] Fix spotBugs error; code review improvements. * [AD-1017] Ignore IO exceptions when reading spawned processes. * Commit Code Coverage Badge * [AD-1017] Update dependency for mongo to 4.8.1 * Commit Code Coverage Badge * [AD-1017] Attempt improve stability of SSH Tunnel client tests. * [AD-1017] Attempt improve stability of multiprocess connection test. * Commit Code Coverage Badge * Update src/main/java/software/amazon/documentdb/jdbc/DocumentDbConnectionProperties.java Co-authored-by: Alexey Temnikov <[email protected]> * [AD-1017] Properly rename parameter. * Commit Code Coverage Badge * [AD-1017] Fix issue with output not being sent to the console (i.e., change level from INFO to ERROR). * Commit Code Coverage Badge * [AD-1017] Attempt to increase code coverage. * [AD-1017] Fix style issue. * Commit Code Coverage Badge Co-authored-by: birschick-bq <[email protected]> Co-authored-by: Alexey Temnikov <[email protected]>
* [AD-1039] Updated release version and dependencies. * Commit Code Coverage Badge Co-authored-by: birschick-bq <[email protected]>
…ws#468) * [Diagnosis] attempt to diagnose test failure * [Diagnosis] attempt to diagnose test failure * Commit Code Coverage Badge * [AD-1043] Mark tests so they only run when integration testing. * Commit Code Coverage Badge * [AD-1043] Attempt to resolve "Commit Code Coverage Badge" when running mavenFilesPreparation * [AD-1043] Add upload of raw-test-results. * [AD-1043] attempt to resolve bug. * Commit Code Coverage Badge Co-authored-by: birschick-bq <[email protected]>
* The schema metadata service can now be changed. A supplier can be passed in when the Driver is created.
Hi, there are 51 files shown in the files tab for this PR, could you please rebase or merge from the |
@alinaliBQ It looks like @birschick-bq already fixed up my branch. It now shows 16 files changed. I'm fine iterating on this PR with feedback. To begin with though, can you look over the PR and see if the overall design makes sense. I'd like to start planning the code that will depend on this. |
Thanks for creating the PR and putting it together, but I'm not primarily working on DocumentDB, perhaps other team members can have a look. |
@normanj-bitquill I'll have a look at the changes/design over the next week. |
@birschick-bq Have you had a chance to look over this PR? |
* The schema metadata service can now be changed. A supplier can be passed in when the Driver is created.
337f412
to
b7e4346
Compare
b7e4346
to
e7998c8
Compare
src/main/java/software/amazon/documentdb/jdbc/metadata/DocumentDbSchema.java
Show resolved
Hide resolved
* Constructs a new DocumentDbDriver with the provided metadata service. | ||
* @param metadataServiceProvider metadata service to use | ||
*/ | ||
public DocumentDbDriver(final Supplier<DocumentDbMetadataService> metadataServiceProvider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normanj-bitquill I'm assuming this will be the entry point to override the schema service for a future local file implementation. Correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@birschick-bq That is correct. The goal is to supply my own implementation of the DocumentDbMetadataService that will be used for all connections made using the driver.
src/main/java/software/amazon/documentdb/jdbc/DocumentDbConnection.java
Outdated
Show resolved
Hide resolved
throws SQLException { | ||
super(connectionProperties); | ||
this.connectionProperties = connectionProperties; | ||
this.metadataService = metadataService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might have to check for nulls since the public interface from DocumentDbDriver
might return a null
from the Supplier
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be sufficient to add a not null check in the DocumentDbDriver
constructor so that the DocumentDbMetadataService
is never null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a null check in DocumentDbDriver
just before this constructor is called.
@sunnie629 @CassieLyu Can you take a look at this PR? I'd like to help move this forward. |
@sunnie629 @CassieLyu how can we help get this PR merged? @normanj-bitquill has responded to all conversations. |
@normanj-bitquill @jlarue26 our product team is working on taking a look at this PR to sign off if we want to merge it |
Much appreciated @sunnie629 |
just to be clear, no custom implementation of the metadata service has been done yet right? this is just changing the code to set that up? |
@sunnie629 I have worked on a custom implementation. The custom implementation would live in a different project that uses this driver. It will be adjusted as needed based on this PR. In short we have a server that maintains its own view of the metadata, so there is no reason for us to write it to the MongoDB database. |
Summary
Want to be able to choose an implementation of DocumentDBSchemaService.
Description
Working on an application that manages collection metadata internally. As a result, it is not desirable to store the collections schemas in the database. The changes here make it possible to replace the implementation of DocumentDBSchemaService.
Related Issue
None
Additional Reviewers
@affonsoBQ
@alinaliBQ
@andiem-bq
@alexey-temnikov
@birschick-bq