-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add utility classes for database object creation #12445
Conversation
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.
LGTM! Left a few small comments but I don't think any of them are blocking
*/ | ||
public static DataSource create(final String username, | ||
final String password, | ||
final String driverClassName, |
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 make sense to create an enum with all of the driver class name values that we support (i.e. the keys in the JDBC_URL_FORMATS
below), and use that enum as the parameter type here? This way we ensure at compile time that these methods are only called with supported class names.
I guess this probably isn't a huge deal if we will be removing this class anyway once we switch to DI
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.
@lmossman Definitely. I'm currently dealing with strings all over the place in the code as I do the refactor that uses these utilities. I will introduce that change as part of that PR.
|
||
private Map<String, String> connectionProperties = Map.of(); | ||
private String database; | ||
private String driverClassName = "org.postgresql.Driver"; |
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.
Just curious, what is the reasoning for setting the default value of this property to postgres?
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.
Almost all of the Database
objects that get created are for Postgresql -- especially those done via the createPostgresqlDatabaseX
methods in the Databases
class. If we feel strongly enough, I can remove this and just throw an exception if the driver class name is not provided.
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 should limit ourself with what we need to use. It will be also quite hard to have an exhaustive list here.
* instances. This class will be removed once the project has been converted to leverage an | ||
* application framework to manage the creation and injection of {@link Flyway} objects. | ||
* | ||
* This class replaces direct calls to {@link io.airbyte.db.Databases}. |
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.
This class replaces direct calls to {@link io.airbyte.db.Databases}.
Is this true? I looked at the Databases class and I didn't see any mention of Flyaway
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.
It's tangentially true. While the the Databases
class doesn't create Flyway
objects, it is currently used to create the Database
object from which a DataSource
instance is retrieved to configure the Flyway
object. See https://github.com/airbytehq/airbyte/blob/master/airbyte-db/lib/src/main/java/io/airbyte/db/instance/FlywayDatabaseMigrator.java#L60 for details.
assertNotNull(dataSource); | ||
assertEquals(HikariDataSource.class, dataSource.getClass()); |
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.
Is it worth also trying to test here that the connectionProperties were set as expected?
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 will see if I can access those properties to confirm configuration. Will do this in my other PR that is providing the refactoring.
|
||
private DataSourceBuilder() {} | ||
|
||
public DataSourceBuilder withConnectionProperties(final Map<String, String> connectionProperties) { |
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.
it is already written, but the @Builder
annotation of lombok can help you writing builders without having to implements them yourself.
return this; | ||
} | ||
|
||
public DataSource build() { |
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.
Oh, I see now what using lombok might be hard here.
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.
This builder may shift some as I do the actual refactor. Once it is finalized, we can revisit it to see if it makes sense to leverage Lombok at that point.
* Add utility classes for database object creation * Remove unused variable
What
How
Recommended reading order
*Factory.java
Databases.java