exceptionfactory commented on pull request #4788: URL: https://github.com/apache/nifi/pull/4788#issuecomment-779460977
> I think this is a good change, but the behavior of a couple of public methods in util classes has changed and a a class was renamed. These could be reasonably be being used by users, so I think it'd be useful to have these deprecated for a cycle with a call out in the release/upgrade notes. Specifically: > > 1. I think it's probably ok to change `isSame()`. It's rather unlikely for a difference to surface. > > 2. It'd be good to leave a `@Deprecated` `computeMd5Digest()` for the 1.14 cycle. > > 3. I'm torn on `generateAdditionalUrlsFingerprint` since it's intended use is pretty specific and I don't think it's as likely that there are users of it. > > 4. I think we can just deprecate `MD5SumMonitor` and add your new `DigestUpdateMonitor`. > > > Since it's (2) and (4) don't require any work, I think we should leave them around for a bit. The tough one is `generateAdditionalUrlsFingerprint`. Thanks for the thoughtful feedback @jfrazee! The points noted raise some good questions about the expectation of compatibility in various NiFi modules. For instance, `MD5SumMonitor` is used exclusively in unit tests within the code base. Aside from the usage of `computeMd5Digest()` in the `isSame()` method, the only usage of that method was in tests. For that method in particular, it seems that it would have been better to mark it as private from the beginning. Regarding `generateAdditionalUrlsFingerprint()`, the return value is now an SHA-256 digest instead of an MD5 digest, but the concept of computing a unique fingerprint from the provided values is unchanged. Unfortunately the method was not documented and did not otherwise indicate that an MD5 digest would be returned. It would be helpful to get some input from additional maintainers, but if all public methods in any class within NiFi should be considered part of the official semantic version, then it seems that more rigor is necessary when creating things like utility classes. I'm open to marking `computeMd5Digest` as deprecated if absolutely necessary, but would prefer to leave the other changes. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
