NIFI-11631 Add OracleDB support for Nifi Registry #7338
Conversation
|
Thanks for this! I tried this at one point (oracle_registry) but had trouble setting up integration tests using testcontainers. I think we really need integration tests here like we have with the other implementation(s). Feel free to compare your PR against my aforementioned commit to make sure we are on the same page. I tested mine manually and everything worked fine, but I never got the integration test stuff working. |
|
Thanks @mattyb149 for bringing your change to my attention. Will definitely check it out. I was not aware that this was already WIP. I was also not aware of existing ITs related to DB in Nifi Registry. I'll try to spin up some tests for OracleDB |
|
Looking good! Needs to be rebased against the latest main to resolve merge conflicts though |
There was a problem hiding this comment.
On a quick review, there is a problem with the explicit reference to the Oracle Data Source class. The Oracle JDBC driver should not be a compilation dependency, and the instanceof checks are not suitable to determine Oracle usage at runtime, so a different approach is necessary before this can go forward.
| <dependency> | ||
| <groupId>com.oracle.database.jdbc</groupId> | ||
| <artifactId>ojdbc8</artifactId> | ||
| <version>23.2.0.0</version> |
There was a problem hiding this comment.
This dependency should have the test scope.
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import oracle.jdbc.datasource.OracleCommonDataSource; |
There was a problem hiding this comment.
Referencing this specific class is a problem for compatibility with other vendors, so a different approach is necessary to check for Oracle as the version.
| final String sql = "SELECT * FROM FLOW_SNAPSHOT WHERE flow_id = ? ORDER BY version DESC LIMIT 1"; | ||
|
|
||
| final String sql; | ||
| if (jdbcTemplate.getDataSource() instanceof OracleCommonDataSource) { |
There was a problem hiding this comment.
A different approach is needed to check for the use of Oracle.
There was a problem hiding this comment.
I was pretty sure this is not the right way to do it and I have seen throughout the code that Flyway's DatabaseTypeRegister.getDatabaseTypeForConnection(connection) was used to get the database type. However, opening then closing the connection just to determine the database type, then have jdbcTemplate open another connection to execute the query have not felt right either. I guess it's not that big of an issue, especially if the data source is a connection pool, and go with the Flyway approach?
There was a problem hiding this comment.
I think the bigger issue here is we really don't want to be checking database types through out the code and having to issue different SQL statements. So even if we did not directly reference the Oracle class here, I would still be against needing to check DB type.
We have a similar problem with this MSSQL PR:
#7245
If there is absolutely no way to make Oracle and MSSQL confirm to the standard SQL that is already used, then I think a more holistic solution may be needed, possible ideas:
- Consider an ORM, we can't use Hibernate because of licensing, could consider EclipseLink
- Create our own ORM like layer for producing SQL statements where we detect the DB type once during start up and based on that we create some kind SQL statement factory based on DB type.
There was a problem hiding this comment.
@bbende Even though I agree that the best solution would be an ORM or a new layer, I'm not sure it should be in the scope of this PR (or in the MSSQL PR's). There would be many conflict for #7245 if this is merged first and vice-versa. We've basically implemented the same things with a different approach already. Maybe it would be worth considering the proper abstraction and merging it first?
There was a problem hiding this comment.
@dam4rus I think we need to take a step back, as we cannot go forward with either Oracle or MSSQL as they stand. This pull request would introduce a direct dependency on Oracle, which is not an option.
It sounds like the best approach is to take a step back as @bbende suggested and consider an abstraction approach, then come back to consider Oracle and MSSQL using that abstraction.
There was a problem hiding this comment.
Yes, sorry I did not mean that this PR should introduce an ORM or abstraction layer. I meant that maybe we should take a step back and consider what options we have for producing DB specific SQL, and try to put that in place before we move forward with Oracle or MSSQL.
There was a problem hiding this comment.
Did you have to add the Oracle-specific stuff just for the tests? IIRC my branch was working with Oracle and I just couldn't get the integration tests working. Of course we want tests in there, but if that's the only issue I would think we could get we 3 reviewers to test it manually and go back to adding tests later if/when it's better supported?
There was a problem hiding this comment.
@mattyb149 No, they are not only for the tests. These sql queries are executed via some REST API calls and that's why they were eluded during my initial manual testing.
@bbende No, I'm sorry because I've just misinterpreted your comment. Sounds good to me.
@exceptionfactory I've actually got rid of directly depending on Oracle. Even though this PR won't be merged as it is I'll push my recent commit to fix that and be more consistent with #7245 by using Flyway instead.
Refactored UnsecuredNifiRegistryClientIT to smaller tests
|
Thanks again for the work on this feature @dam4rus. Based on the discussion, I created a new Jira issue NIFI-12059 describing a general need for Registry framework adjustments, along with a potential solution. Once such a solution is in place, then we should be in a better position in implement support for additional database vendors, so I'm closing this pull request for now. |
Tested with Oracle Database version 19. Migrating, bucket creation, version controlling flows all worked fine for me. Will probably need a more thorough testing.
Summary
NIFI-11631
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation