Repository navigation
Addition of Databricks Plugin - #680
vikasrathee-cs wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new Databricks plugin module, including a batch source, database connector, configuration classes, documentation, widgets, and unit tests. It also updates the database-commons module to support custom auto-commit and transaction isolation levels. The review feedback highlights several critical improvements: preventing potential NullPointerExceptions when the connection configuration or column names are null, handling unsupported transaction isolation levels gracefully instead of throwing RuntimeExceptions, and implementing actual random sampling in the getRandomQuery method using ORDER BY rand().
80cabb2 to
9a4d4fc
Compare
9a4d4fc to
e865f6d
Compare
|
Hi, I don't think you have pushed the changes yet . |
e865f6d to
7057300
Compare
7057300 to
de3fc24
Compare
de3fc24 to
b499316
Compare
| "label": "Password / Token", | ||
| "name": "password" | ||
| }, | ||
| { |
There was a problem hiding this comment.
Since databricks-jdbc 3.4.3 only supports TRANSACTION_REPEATABLE_READ, offering "TRANSACTION_READ_UNCOMMITTED" here (and in Databricks-connector.json on line 53) will cause Batch Source pipeline runs to fail at runtime in DataDrivenETLDBInputFormat.getConnection() when connection.setTransactionIsolation(...) throws a SQLException. Please either restrict values to ["TRANSACTION_REPEATABLE_READ"] or remove the dropdown widget from both JSON files.
There was a problem hiding this comment.
I verified the behavior with JDBC 2.6.40: it reports TIMESTAMP_NTZ columns as TIMESTAMP through getColumnTypeName(), so the plugin can’t distinguish them from regular timestamps. JDBC 3.4.3 reports TIMESTAMP_NTZ correctly, which lets the plugin map it as a local datetime. Since 2.6.40 accepts TRANSACTION_READ_UNCOMMITTED but rejects TRANSACTION_REPEATABLE_READ (error: unsupported isolation level 4), while 3.4.3 requires TRANSACTION_REPEATABLE_READ, both isolation options are needed. I’ve documented the version-specific guidance in the batch source and connector docs.
| } | ||
|
|
||
| @Override | ||
| public Map<String, String> getAdditionalArguments() { |
There was a problem hiding this comment.
Overriding getAdditionalArguments() to put TransactionIsolationLevel.CONF_KEY is redundant because AbstractDBSpecificConnector.getInputFormatProvider() already calls connectionConfigAccessor.setTransactionIsolationLevel(getTransactionIsolationLevel()). Moreover, in DatabricksSource, getAdditionalArguments() is merged into the JDBC Properties passed to DriverManager.getConnection(url, properties), which unnecessarily passes mapreduce.jdbc.isolation.level as a JDBC driver connection property. You can remove this override.
If this needed, please justify.
There was a problem hiding this comment.
I removed the redundant getAdditionalArguments() override. The selected isolation level is still set directly through setTransactionIsolationLevel() and applied with Connection.setTransactionIsolation(), so the change preserves the configured isolation level.
sahusanket
left a comment
There was a problem hiding this comment.
Minor comments left.
Please address before merging.
Addition of Databricks Plugin