Skip to content
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

NIFI-11946 Added preserveSourceProperties query parameter to create flow API #7604

Closed
wants to merge 10 commits into from

Conversation

ravinarayansingh
Copy link
Contributor

Summary

NIFI-11946

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • 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 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@ravinarayansingh ravinarayansingh changed the title Added preserveSourceProperties query parameter to create flow API NIFI-11946 Added preserveSourceProperties query parameter to create flow API Aug 14, 2023
Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @ravinarayansingh.

On initial review, supporting this feature raises some concerns. The created and modified timestamps are intended to reflect what actually occurred. With this optional flag, however, there is no indication that the timestamps do not reflect when a flow was actually created or modified. For this reason, supporting this option seems like it could raise potential usability and tracking issues.

Do you have any additional background for the motivation behind this change?

@ravinarayansingh
Copy link
Contributor Author

Thanks for the contribution @ravinarayansingh.

On initial review, supporting this feature raises some concerns. The created and modified timestamps are intended to reflect what actually occurred. With this optional flag, however, there is no indication that the timestamps do not reflect when a flow was actually created or modified. For this reason, supporting this option seems like it could raise potential usability and tracking issues.

Do you have any additional background for the motivation behind this change?

Hi @exceptionfactory

I'm looking to upgrade a NiFi Registry that currently uses h2 and the Filesystem as a persistence provider. My goal is to enhance the registry by migrating to Postgres as the provider. During this transition, I need to ensure that the flows from the old registry are transferred to the new one while retaining their original authors and timestamps.

The issue I've encountered is that the current behavior of the "create flow version" API only preserves the author information but not the timestamp. As a solution, I've submitted a pull request (PR) that addresses this concern.

Thanks

@exceptionfactory
Copy link
Contributor

Thanks for the contribution @ravinarayansingh.
On initial review, supporting this feature raises some concerns. The created and modified timestamps are intended to reflect what actually occurred. With this optional flag, however, there is no indication that the timestamps do not reflect when a flow was actually created or modified. For this reason, supporting this option seems like it could raise potential usability and tracking issues.
Do you have any additional background for the motivation behind this change?

Hi @exceptionfactory

I'm looking to upgrade a NiFi Registry that currently uses h2 and the Filesystem as a persistence provider. My goal is to enhance the registry by migrating to Postgres as the provider. During this transition, I need to ensure that the flows from the old registry are transferred to the new one while retaining their original authors and timestamps.

The issue I've encountered is that the current behavior of the "create flow version" API only preserves the author information but not the timestamp. As a solution, I've submitted a pull request (PR) that addresses this concern.

Thanks

Thanks for the additional background @ravinarayansingh, that is very helpful.

In that scenario, the best approach would be to export the information from H2 and import it to PostgreSQL. The H2 Tutorial provides some basic guidance on loading a H2 file, which could then be used for exporting. There will be some differences between H2 and PostgreSQL syntax, but this would be a better way to handle such a migration.

The create flow REST methods are not necessarily intended for bulk migration, so from that perspective, introducing this new parameter does not seem like the best solution.

@ravinarayansingh
Copy link
Contributor Author

ravinarayansingh commented Aug 22, 2023

Thanks for the contribution @ravinarayansingh.
On initial review, supporting this feature raises some concerns. The created and modified timestamps are intended to reflect what actually occurred. With this optional flag, however, there is no indication that the timestamps do not reflect when a flow was actually created or modified. For this reason, supporting this option seems like it could raise potential usability and tracking issues.
Do you have any additional background for the motivation behind this change?

Hi @exceptionfactory
I'm looking to upgrade a NiFi Registry that currently uses h2 and the Filesystem as a persistence provider. My goal is to enhance the registry by migrating to Postgres as the provider. During this transition, I need to ensure that the flows from the old registry are transferred to the new one while retaining their original authors and timestamps.
The issue I've encountered is that the current behavior of the "create flow version" API only preserves the author information but not the timestamp. As a solution, I've submitted a pull request (PR) that addresses this concern.
Thanks

Thanks for the additional background @ravinarayansingh, that is very helpful.

In that scenario, the best approach would be to export the information from H2 and import it to PostgreSQL. The H2 Tutorial provides some basic guidance on loading a H2 file, which could then be used for exporting. There will be some differences between H2 and PostgreSQL syntax, but this would be a better way to handle such a migration.

The create flow REST methods are not necessarily intended for bulk migration, so from that perspective, introducing this new parameter does not seem like the best solution.

Hi @exceptionfactory
thanks for response

As we know create bucket , create flow version both API's already have preserveSourceProperties parameter and for this one create Flow i have added .

While I acknowledge that the current approach might not be optimal, I'm curious about the initial rationale for including this parameter. Additionally, could you clarify the reasoning behind exclusively maintaining the author's information?

@exceptionfactory
Copy link
Contributor

Hi @exceptionfactory thanks for response

As we know create bucket , create flow version both API's already have preserveSourceProperties parameter and for this one create Flow i have added .

While I acknowledge that the current approach might not be optimal, I'm curious about the initial rationale for including this parameter. Additionally, could you clarify the reasoning behind exclusively maintaining the author's information?

Thanks for the reply @ravinarayansingh, that is a good point and question.

NIFI-11327 introduced the preserveSourceProperties parameter as part of supporting exporting and import operations. From that perspective, there are several create methods that support these operations, so my previous statement was not accurate.

Given that the basic purpose of this parameter is to support some types of migration, the question whether to extend the implementation to timestamps, instead of keeping it limited to the author. Preserving the created timestamp might make sense to maintain historical tracking, but it seems like the modified timestamp should reflect the time when the migration operation occurred.

Is there some particular reason for not wanting to reflect the current timestamp when importing flows?

@ravinarayansingh
Copy link
Contributor Author

Is there some particular reason for not wanting to reflect the current timestamp when importing flows?

Thanks @exceptionfactory for quick response

No, there is no specific reason and you are right I should set ModifiedTimestamp with current timestamp.
if you are agree then I can update the PR with required changes

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ravinarayansingh, making adjustments to only retain the created timestamp, but always update the modified timestamp, seems like the best way forward. I also noted a couple minor spacing adjustments, and a recommendation for how to handle checking for and passing down the existing created timestamp.

ravinarayansingh and others added 6 commits August 23, 2023 15:09
…n/java/org/apache/nifi/registry/web/service/StandardServiceFacade.java

Co-authored-by: exceptionfactory <exceptionfactory@apache.org>
…n/java/org/apache/nifi/registry/web/service/StandardServiceFacade.java

Co-authored-by: exceptionfactory <exceptionfactory@apache.org>
…n/java/org/apache/nifi/registry/web/api/BucketFlowResource.java

Co-authored-by: exceptionfactory <exceptionfactory@apache.org>
…ain/java/org/apache/nifi/registry/service/RegistryService.java

Co-authored-by: exceptionfactory <exceptionfactory@apache.org>
…n/java/org/apache/nifi/registry/web/api/BucketFlowResource.java

Co-authored-by: exceptionfactory <exceptionfactory@apache.org>
@ravinarayansingh
Copy link
Contributor Author

Thanks @ravinarayansingh, making adjustments to only retain the created timestamp, but always update the modified timestamp, seems like the best way forward. I also noted a couple minor spacing adjustments, and a recommendation for how to handle checking for and passing down the existing created timestamp.

hi @exceptionfactory I have made the changes please have a look
thanks

@ravinarayansingh
Copy link
Contributor Author

Hi @exceptionfactory
let me know if I have to do any changes from my side

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the adjustments and working through the feedback @ravinarayansingh, the latest version looks good. +1 merging

exceptionfactory pushed a commit that referenced this pull request Aug 28, 2023
This closes #7604

Signed-off-by: David Handermann <exceptionfactory@apache.org>
(cherry picked from commit 6bb2d62)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants