You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "dam4rus (via GitHub)" <gi...@apache.org> on 2023/06/05 13:05:46 UTC
[GitHub] [nifi] dam4rus opened a new pull request, #7338: NIFI-11631 Add OracleDB support for Nifi Registry
dam4rus opened a new pull request, #7338:
URL: https://github.com/apache/nifi/pull/7338
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
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](https://issues.apache.org/jira/browse/NIFI-11631)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 11
- [x] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7338:
URL: https://github.com/apache/nifi/pull/7338#discussion_r1228650340
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/db/DatabaseMetadataService.java:
##########
@@ -16,7 +16,21 @@
*/
package org.apache.nifi.registry.db;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import oracle.jdbc.datasource.OracleCommonDataSource;
Review 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.
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/db/DatabaseMetadataService.java:
##########
@@ -391,8 +391,12 @@ public FlowSnapshotEntity getFlowSnapshot(final String flowIdentifier, final Int
@Override
public FlowSnapshotEntity getLatestSnapshot(final String flowIdentifier) {
- final String sql = "SELECT * FROM FLOW_SNAPSHOT WHERE flow_id = ? ORDER BY version DESC LIMIT 1";
-
+ final String sql;
+ if (jdbcTemplate.getDataSource() instanceof OracleCommonDataSource) {
Review Comment:
A different approach is needed to check for the use of Oracle.
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/pom.xml:
##########
@@ -420,6 +420,11 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>com.oracle.database.jdbc</groupId>
+ <artifactId>ojdbc8</artifactId>
+ <version>23.2.0.0</version>
Review Comment:
This dependency should have the `test` scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] bbende commented on a diff in pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "bbende (via GitHub)" <gi...@apache.org>.
bbende commented on code in PR #7338:
URL: https://github.com/apache/nifi/pull/7338#discussion_r1229979292
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/db/DatabaseMetadataService.java:
##########
@@ -391,8 +391,12 @@ public FlowSnapshotEntity getFlowSnapshot(final String flowIdentifier, final Int
@Override
public FlowSnapshotEntity getLatestSnapshot(final String flowIdentifier) {
- final String sql = "SELECT * FROM FLOW_SNAPSHOT WHERE flow_id = ? ORDER BY version DESC LIMIT 1";
-
+ final String sql;
+ if (jdbcTemplate.getDataSource() instanceof OracleCommonDataSource) {
Review 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:
https://github.com/apache/nifi/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7338:
URL: https://github.com/apache/nifi/pull/7338#discussion_r1230016163
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/db/DatabaseMetadataService.java:
##########
@@ -391,8 +391,12 @@ public FlowSnapshotEntity getFlowSnapshot(final String flowIdentifier, final Int
@Override
public FlowSnapshotEntity getLatestSnapshot(final String flowIdentifier) {
- final String sql = "SELECT * FROM FLOW_SNAPSHOT WHERE flow_id = ? ORDER BY version DESC LIMIT 1";
-
+ final String sql;
+ if (jdbcTemplate.getDataSource() instanceof OracleCommonDataSource) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] dam4rus commented on a diff in pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "dam4rus (via GitHub)" <gi...@apache.org>.
dam4rus commented on code in PR #7338:
URL: https://github.com/apache/nifi/pull/7338#discussion_r1229023824
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/db/DatabaseMetadataService.java:
##########
@@ -391,8 +391,12 @@ public FlowSnapshotEntity getFlowSnapshot(final String flowIdentifier, final Int
@Override
public FlowSnapshotEntity getLatestSnapshot(final String flowIdentifier) {
- final String sql = "SELECT * FROM FLOW_SNAPSHOT WHERE flow_id = ? ORDER BY version DESC LIMIT 1";
-
+ final String sql;
+ if (jdbcTemplate.getDataSource() instanceof OracleCommonDataSource) {
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on a diff in pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "mattyb149 (via GitHub)" <gi...@apache.org>.
mattyb149 commented on code in PR #7338:
URL: https://github.com/apache/nifi/pull/7338#discussion_r1230126523
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/db/DatabaseMetadataService.java:
##########
@@ -391,8 +391,12 @@ public FlowSnapshotEntity getFlowSnapshot(final String flowIdentifier, final Int
@Override
public FlowSnapshotEntity getLatestSnapshot(final String flowIdentifier) {
- final String sql = "SELECT * FROM FLOW_SNAPSHOT WHERE flow_id = ? ORDER BY version DESC LIMIT 1";
-
+ final String sql;
+ if (jdbcTemplate.getDataSource() instanceof OracleCommonDataSource) {
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] dam4rus commented on a diff in pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "dam4rus (via GitHub)" <gi...@apache.org>.
dam4rus commented on code in PR #7338:
URL: https://github.com/apache/nifi/pull/7338#discussion_r1230479039
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/db/DatabaseMetadataService.java:
##########
@@ -391,8 +391,12 @@ public FlowSnapshotEntity getFlowSnapshot(final String flowIdentifier, final Int
@Override
public FlowSnapshotEntity getLatestSnapshot(final String flowIdentifier) {
- final String sql = "SELECT * FROM FLOW_SNAPSHOT WHERE flow_id = ? ORDER BY version DESC LIMIT 1";
-
+ final String sql;
+ if (jdbcTemplate.getDataSource() instanceof OracleCommonDataSource) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] dam4rus commented on a diff in pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "dam4rus (via GitHub)" <gi...@apache.org>.
dam4rus commented on code in PR #7338:
URL: https://github.com/apache/nifi/pull/7338#discussion_r1230007521
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/db/DatabaseMetadataService.java:
##########
@@ -391,8 +391,12 @@ public FlowSnapshotEntity getFlowSnapshot(final String flowIdentifier, final Int
@Override
public FlowSnapshotEntity getLatestSnapshot(final String flowIdentifier) {
- final String sql = "SELECT * FROM FLOW_SNAPSHOT WHERE flow_id = ? ORDER BY version DESC LIMIT 1";
-
+ final String sql;
+ if (jdbcTemplate.getDataSource() instanceof OracleCommonDataSource) {
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] dam4rus commented on pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "dam4rus (via GitHub)" <gi...@apache.org>.
dam4rus commented on PR #7338:
URL: https://github.com/apache/nifi/pull/7338#issuecomment-1578038320
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "mattyb149 (via GitHub)" <gi...@apache.org>.
mattyb149 commented on PR #7338:
URL: https://github.com/apache/nifi/pull/7338#issuecomment-1577116427
Thanks for this! I tried this at one point ([oracle_registry](https://github.com/mattyb149/nifi/commit/e912b9155cce0b919f0a3d4d9edfc55c7038a65b)) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #7338:
URL: https://github.com/apache/nifi/pull/7338#issuecomment-1719937128
Thanks again for the work on this feature @dam4rus. Based on the discussion, I created a new Jira issue [NIFI-12059](https://issues.apache.org/jira/browse/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "mattyb149 (via GitHub)" <gi...@apache.org>.
mattyb149 commented on PR #7338:
URL: https://github.com/apache/nifi/pull/7338#issuecomment-1589920349
Looking good! Needs to be rebased against the latest main to resolve merge conflicts though
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] bbende commented on a diff in pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "bbende (via GitHub)" <gi...@apache.org>.
bbende commented on code in PR #7338:
URL: https://github.com/apache/nifi/pull/7338#discussion_r1230033355
##########
nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/db/DatabaseMetadataService.java:
##########
@@ -391,8 +391,12 @@ public FlowSnapshotEntity getFlowSnapshot(final String flowIdentifier, final Int
@Override
public FlowSnapshotEntity getLatestSnapshot(final String flowIdentifier) {
- final String sql = "SELECT * FROM FLOW_SNAPSHOT WHERE flow_id = ? ORDER BY version DESC LIMIT 1";
-
+ final String sql;
+ if (jdbcTemplate.getDataSource() instanceof OracleCommonDataSource) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory closed pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #7338: NIFI-11631 Add OracleDB support for Nifi Registry
URL: https://github.com/apache/nifi/pull/7338
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org