[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1042574240 Thanks adding the license and updating the notice information @tpalfy. With those changes, this looks just about complete. Two recent updates for Commons Lang3 (#5773) and Commons DBCP (#5763) standardized the versioning of those dependencies and removed the need to declare those versions explicitly. I can update the branch to double-check the build if there are no other changes necessary. Do you have any additional feedback @turcsanyip? Otherwise I make the dependency adjustments to confirm a successful build and then merge. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1033105888 One other point of consideration, the `snowflake-jdbc` JAR is almost 28 MB due to shading a large number of dependencies. In light of current sizing limitations on the standard NiFi binary, it seems like this should not be part of the standard assembly. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1022273920 The `getDriver()` method appears to be exactly the same as the `DBCPConnectionPool`, so perhaps I am missing how duplicating it changes the runtime behavior. Although there may be some marginal utility in having an explicit controller service, I am not in favor of the current approach without more significant code reduction of duplication. If that requires some refactoring of `DBCPConnectionPool` to enable clean extension, that seems better than duplicating property descriptors and configuration methods. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1021522662 Is there a reason this class is unable to use the `getDriver()` method from `DBCPConnectionPool`? The methods appear to be the same. Although there may not be business logic duplication in `onConfigured()`, there is still a lot of setup code that should be shared. The same applies to the property descriptor definitions. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1020456025 -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1021271665 Thanks @turcsanyip, that sounds like a good solution! -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1020481969 There is a good deal of code duplication in certain places right now, so this is not necessarily so different, but it is an opportunity to find a better approach. The idea of a `nifi-dbcp-service-meta` sounds a good potential solution. I agree this does not follow the usual pattern of a utility module, so other approaches are certainly worth considering. Moving the META-INF/services definition to a separate module would allow a `nifi-snowflake-service` module to depend on `nifi-dbcp-service`, so that sounds like a workable solution. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1020456025 The `nifi-dbcp-shared` would contain the shared service implementation code, but not the service definition file, so that would be the difference from the current structure. This is somewhat of an odd case since it is essentially providing a convenience class for something that can be configured using the existing controller service. Although there may not be good examples of this in the current code base, this is an opportunity to avoid unnecessary code duplication. There may be other module structure alternatives, but one way or the other, we should avoid this level of code duplication. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1020179087 Thanks for following up on this @tpalfy. Having a NAR dependency can be problematic because it could pull in the META-INF directory with the Processor definition, which would result in the attempt to register the same processor more than once. Excluding specific files from the destination NAR becomes problematic as well. It seems like the best approach would require refactoring shared code to a new module, and then depending on that module in both `nifi-dbcp-service` and `nifi-snowflake-service`. Perhaps named something like `nifi-dbcp-shared`? That module would contain the shared code, but would not include the META-INF directory. That type of approach should avoid potential issues that would result by attempting to depend on the existing DBCP JAR or NAR modules as currently defined. That will require some refactoring, but it seems like the best approach when it comes to avoiding duplication. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1017835872 Thanks for the background @joewitt, that is understandable, although it seems like the same argument could be made for any number of JDBC-based services. The code duplication is a pain point, so I would be more favorable to this change if the implementation inherited from the standard DBCPConnectionPool service. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #5692: NIFI-9609 Added nifi-snowflake-bundle with a SnowflakeConnectionPool.
exceptionfactory commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1017815068 This PR appears to introduce a large amount of code duplication from the standard DBCPConnectionPool service just to register the Snowflake Driver. Does standard DBCPConnectionPool service not work with the Snowflake Computing JDBC driver? -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
