You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/09/10 01:43:24 UTC

[GitHub] [nifi] exceptionfactory opened a new pull request, #6390: NIFI-10471 Document and apply standard deprecation logging

exceptionfactory opened a new pull request, #6390:
URL: https://github.com/apache/nifi/pull/6390

   # Summary
   
   [NIFI-10471](https://issues.apache.org/jira/browse/NIFI-10471) Updates the Administrator's Guide and Developer Guide with configuration and implementation details for component and feature deprecation logging. Additional deprecation logging includes warning when a component with a `DeprecationNotice` is added to the Flow Configuration, and warning when several components use deprecated methods or properties.
   
   The following components include specific usage deprecation warnings:
   
   - `HDFSNarProvider` class
   - `InvokeHTTP` proxy properties
   - `ListHDFS` distributed cache property
   - `PersistentProvenanceRepository` class
   - `RocksDBFlowFileRepository` class
   - `RecordSetWriterFactory` interface methods
   - `SchemaRegistry` interface methods
   - `StandardSSLContextService` interface methods
   
   Additional changes include removing integration test class for the deprecated `PersistentProvenanceRepository` class.
   
   # 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
   
   - [X] Build completed using `mvn clean install -P contrib-check`
     - [X] JDK 8
     - [X] 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
   
   - [X] 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 pull request #6390: NIFI-10471 Document and apply standard deprecation logging

Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6390:
URL: https://github.com/apache/nifi/pull/6390#issuecomment-1245932579

   Thanks for the review and feedback @greyp9! I pushed an update with more logging for the repository class and additional paragraphs in the Administrator's Guide.


-- 
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 #6390: NIFI-10471 Document and apply standard deprecation logging

Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6390:
URL: https://github.com/apache/nifi/pull/6390#discussion_r970043075


##########
nifi-nar-bundles/nifi-provenance-repository-bundle/nifi-persistent-provenance-repository/src/main/java/org/apache/nifi/provenance/PersistentProvenanceRepository.java:
##########
@@ -226,6 +229,8 @@ public PersistentProvenanceRepository(final NiFiProperties nifiProperties) throw
     }
 
     public PersistentProvenanceRepository(final RepositoryConfiguration configuration, final int rolloverCheckMillis) throws IOException {
+        deprecationLogger.warn("{} should be replaced with WriteAheadProvenanceRepository", getClass().getSimpleName());

Review Comment:
   Good point, will extend the logging to include a reference to the specific property.



-- 
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 #6390: NIFI-10471 Document and apply standard deprecation logging

Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6390:
URL: https://github.com/apache/nifi/pull/6390#discussion_r970044501


##########
nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/ListHDFS.java:
##########
@@ -277,6 +281,14 @@ public Set<Relationship> getRelationships() {
 
     @Override
     protected Collection<ValidationResult> customValidate(ValidationContext context) {
+        if (context.getProperty(DISTRIBUTED_CACHE_SERVICE).isSet()) {

Review Comment:
   That's a good point. Some types of deprecation logging will only appear when executing various components and methods. I will look at adding a note to the Administrator's Guide.



-- 
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] greyp9 closed pull request #6390: NIFI-10471 Document and apply standard deprecation logging

Posted by GitBox <gi...@apache.org>.
greyp9 closed pull request #6390: NIFI-10471 Document and apply standard deprecation logging
URL: https://github.com/apache/nifi/pull/6390


-- 
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 #6390: NIFI-10471 Document and apply standard deprecation logging

Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6390:
URL: https://github.com/apache/nifi/pull/6390#discussion_r970039560


##########
nifi-docs/src/main/asciidoc/developer-guide.adoc:
##########
@@ -2637,19 +2637,83 @@ logged to help avoid this bad practice.
 
 
 [[deprecation]]
-== Deprecating a Component
+== Deprecating Components and Features
+Deprecating features is an important part of the software development lifecycle, providing an upgrade path for
+developers and users to follow.
+
+Apache NiFi follows the link:https://semver.org[Semantic Versioning Specification 2.0.0^] for features identified as
+part of the public version contract according to
+link:https://cwiki.apache.org/confluence/display/NIFI/Version+Scheme+and+API+Compatibility[Version Scheme]
+documentation.
+
+Components and features that fit under the public version contract require deprecation marking prior to removal. This
+approach allows developers to implement changes as part of minor version upgrades, in preparation for future removal
+of features in a subsequent major version.

Review Comment:
   In the larger context of this section, this references to removing features that would cause breaking changes. That is the reason for the opening sentence referencing "features that fit under the public version contract." There may be some theoretical edge case where a feature needs to be removed for some other reason, like a security vulnerability, but this sentence states the standard 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] greyp9 commented on a diff in pull request #6390: NIFI-10471 Document and apply standard deprecation logging

Posted by GitBox <gi...@apache.org>.
greyp9 commented on code in PR #6390:
URL: https://github.com/apache/nifi/pull/6390#discussion_r969823088


##########
nifi-docs/src/main/asciidoc/developer-guide.adoc:
##########
@@ -2637,19 +2637,83 @@ logged to help avoid this bad practice.
 
 
 [[deprecation]]
-== Deprecating a Component
+== Deprecating Components and Features
+Deprecating features is an important part of the software development lifecycle, providing an upgrade path for
+developers and users to follow.
+
+Apache NiFi follows the link:https://semver.org[Semantic Versioning Specification 2.0.0^] for features identified as
+part of the public version contract according to
+link:https://cwiki.apache.org/confluence/display/NIFI/Version+Scheme+and+API+Compatibility[Version Scheme]
+documentation.
+
+Components and features that fit under the public version contract require deprecation marking prior to removal. This
+approach allows developers to implement changes as part of minor version upgrades, in preparation for future removal
+of features in a subsequent major version.

Review Comment:
   So are we saying here that removal of features would *only* take place in the context of a major version upgrade?



##########
nifi-docs/src/main/asciidoc/developer-guide.adoc:
##########
@@ -2637,19 +2637,83 @@ logged to help avoid this bad practice.
 
 
 [[deprecation]]
-== Deprecating a Component
+== Deprecating Components and Features
+Deprecating features is an important part of the software development lifecycle, providing an upgrade path for
+developers and users to follow.
+
+Apache NiFi follows the link:https://semver.org[Semantic Versioning Specification 2.0.0^] for features identified as
+part of the public version contract according to
+link:https://cwiki.apache.org/confluence/display/NIFI/Version+Scheme+and+API+Compatibility[Version Scheme]
+documentation.
+
+Components and features that fit under the public version contract require deprecation marking prior to removal. This
+approach allows developers to implement changes as part of minor version upgrades, in preparation for future removal
+of features in a subsequent major version.
+
+=== Component Deprecation
+
 Sometimes it may be desirable to deprecate a component. Whenever this occurs the developer may use the
  `@DeprecationNotice` annotation to indicate that a component has been deprecated, allowing the developer
- to describe a reason for the deprecation and suggest alternative components. An example of how to do this can
+ to describe a `reason` for the deprecation and suggest alternative components. An example of how to do this can
  be found below:
 
 [source, java]
 ----
- @DeprecationNotice(alternatives = {ListenSyslog.class}, classNames = {"org.apache.nifi.processors.standard.ListenRELP"}, reason = "Technology has been superseded",  )
- public class ListenOldProtocol extends AbstractProcessor {
+@DeprecationNotice(
+        reason = "Legacy Protocol has been superseded",
+        alternatives = {ListenSyslog.class},
+        classNames = {"org.apache.nifi.processors.standard.ListenRELP"}
+)
+public class ListenLegacyProtocol extends AbstractProcessor {}
+----
+The `alternatives` property can be used to define an array of recommended replacement Components, while `classNames`
+can be used to represent similar content through an array of class name strings.
+
+Adding the `@DeprecationNotice` annotation renders a warning message in generated documentation and also logs the
+following warning when the Flow Configuration includes the component:
+
+----
+Added Deprecated Component ListenLegacyProtocol[id=929a52c7-1e3e-423e-b303-6ca2ef657617] See alternatives [ListenSyslog,ListenRELP]
+----
+
+=== Feature Deprecation
+
+Deprecating features includes changing component configuration strategies, introducing new repository classes, and
+refactoring a Controller Service interface. Removing component properties can create invalid Flow Configurations after
+upgrading, and removing public methods from a Controller Service interface can break components compiled against
+previous versions. For these reasons, introducing new properties and methods must include a deprecation strategy that
+supports compatibility when upgrading from one minor version to another.
+
+Annotating methods and properties with the Java `@Deprecated` annotation provides a warning to software developers, but

Review Comment:
   +1, ... this suggests a good strategy for deprecation, `@Deprecated` for developers, and deprecation logging for users 



##########
nifi-docs/src/main/asciidoc/administration-guide.adoc:
##########
@@ -182,6 +182,41 @@ Antivirus software can take a long time to scan large directories and the numero
 * `provenance_repository`
 * `state`
 
+[[logging_configuration]]
+== Logging Configuration
+NiFi uses link:https://logback.qos.ch/[logback^] as the runtime logging implementation. The `conf` directory contains a
+standard `logback.xml` configuration with default appender and level settings. The
+link:https://logback.qos.ch/manual/index.html[logback manual] provides a complete reference of available options.
+
+=== Standard Log Files
+The standard logback configuration includes the following appender definitions and associated log files:
+
+[options="header"]
+|=========================
+| File                   | Description
+| `nifi-app.log`         | Application log containing framework and component messages
+| `nifi-bootstrap.log`   | Bootstrap log containing startup and shutdown messages
+| `nifi-deprecation.log` | Deprecation log containing warnings for deprecated components and features
+| `nifi-request.log`     | HTTP request log containing user interface and REST API access messages
+| `nifi-user.log`        | User log containing authentication and authorization messages
+|=========================
+
+=== Deprecation Logging
+The `nifi-deprecation.log` contains warning messages describing components and features that will be removed in
+subsequent versions. Deprecation warnings should be evaluated and addressed to avoid breaking changes when upgrading to
+a new major version. Resolving deprecation warnings involves upgrading to new components, changing component property
+settings, or refactoring custom component classes.
+

Review Comment:
   Suggest additional clarifying wording here along the lines of:
   
   - The deprecation log should be checked before upgrading to the next minor version.
   - To upgrade several minor versions, the deprecation notices in the release notes should be checked for relevant content.
   - To upgrade to the next major version, first upgrade to the latest minor version, and run the flow for some period of time to check for deprecation logs indicating needed corrections.
   



##########
nifi-nar-bundles/nifi-provenance-repository-bundle/nifi-persistent-provenance-repository/src/main/java/org/apache/nifi/provenance/PersistentProvenanceRepository.java:
##########
@@ -226,6 +229,8 @@ public PersistentProvenanceRepository(final NiFiProperties nifiProperties) throw
     }
 
     public PersistentProvenanceRepository(final RepositoryConfiguration configuration, final int rolloverCheckMillis) throws IOException {
+        deprecationLogger.warn("{} should be replaced with WriteAheadProvenanceRepository", getClass().getSimpleName());

Review Comment:
   Should we extend the logging a bit to point users at the right place to make the needed modification?  For example, this is a fix to `nifi.properties`, where the `ListHDFS` change should be made in the processor properties.



##########
nifi-docs/src/main/asciidoc/administration-guide.adoc:
##########
@@ -182,6 +182,41 @@ Antivirus software can take a long time to scan large directories and the numero
 * `provenance_repository`
 * `state`
 
+[[logging_configuration]]
+== Logging Configuration
+NiFi uses link:https://logback.qos.ch/[logback^] as the runtime logging implementation. The `conf` directory contains a
+standard `logback.xml` configuration with default appender and level settings. The
+link:https://logback.qos.ch/manual/index.html[logback manual] provides a complete reference of available options.
+
+=== Standard Log Files
+The standard logback configuration includes the following appender definitions and associated log files:
+
+[options="header"]
+|=========================
+| File                   | Description
+| `nifi-app.log`         | Application log containing framework and component messages
+| `nifi-bootstrap.log`   | Bootstrap log containing startup and shutdown messages
+| `nifi-deprecation.log` | Deprecation log containing warnings for deprecated components and features
+| `nifi-request.log`     | HTTP request log containing user interface and REST API access messages
+| `nifi-user.log`        | User log containing authentication and authorization messages
+|=========================
+
+=== Deprecation Logging
+The `nifi-deprecation.log` contains warning messages describing components and features that will be removed in
+subsequent versions. Deprecation warnings should be evaluated and addressed to avoid breaking changes when upgrading to
+a new major version. Resolving deprecation warnings involves upgrading to new components, changing component property
+settings, or refactoring custom component classes.
+

Review Comment:
   Some clarifying verbiage here might be helpful:
   
   (first draft)
   Before upgrading to a new version of NiFi, it should be a checklist item to run NiFi for $DURATION with an empty `nifi-deprecation.log`.  If the upgrade spans multiple NiFi versions, the upgrade checklist should consider the deprecation notices in each relevant release notes document.  Before a major version upgrade, it is suggested that an intervening upgrade be performed to the latest minor release, in order to evaluate the flow deprecation log for needed adjustments.
   



##########
nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/ListHDFS.java:
##########
@@ -277,6 +281,14 @@ public Set<Relationship> getRelationships() {
 
     @Override
     protected Collection<ValidationResult> customValidate(ValidationContext context) {
+        if (context.getProperty(DISTRIBUTED_CACHE_SERVICE).isSet()) {

Review Comment:
   It looks like disabling a processor with this style of deprecation logging causes the logging to stop.  Not sure where, but that might be a good thing to note... somewhere.



-- 
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 #6390: NIFI-10471 Document and apply standard deprecation logging

Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6390:
URL: https://github.com/apache/nifi/pull/6390#discussion_r970059986


##########
nifi-docs/src/main/asciidoc/administration-guide.adoc:
##########
@@ -182,6 +182,41 @@ Antivirus software can take a long time to scan large directories and the numero
 * `provenance_repository`
 * `state`
 
+[[logging_configuration]]
+== Logging Configuration
+NiFi uses link:https://logback.qos.ch/[logback^] as the runtime logging implementation. The `conf` directory contains a
+standard `logback.xml` configuration with default appender and level settings. The
+link:https://logback.qos.ch/manual/index.html[logback manual] provides a complete reference of available options.
+
+=== Standard Log Files
+The standard logback configuration includes the following appender definitions and associated log files:
+
+[options="header"]
+|=========================
+| File                   | Description
+| `nifi-app.log`         | Application log containing framework and component messages
+| `nifi-bootstrap.log`   | Bootstrap log containing startup and shutdown messages
+| `nifi-deprecation.log` | Deprecation log containing warnings for deprecated components and features
+| `nifi-request.log`     | HTTP request log containing user interface and REST API access messages
+| `nifi-user.log`        | User log containing authentication and authorization messages
+|=========================
+
+=== Deprecation Logging
+The `nifi-deprecation.log` contains warning messages describing components and features that will be removed in
+subsequent versions. Deprecation warnings should be evaluated and addressed to avoid breaking changes when upgrading to
+a new major version. Resolving deprecation warnings involves upgrading to new components, changing component property
+settings, or refactoring custom component classes.
+

Review Comment:
   Thanks for the suggestion, will add some additional details.



-- 
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