NIFI-748 Fixed logic around handling partial query results from prove… #123
Conversation
…nance repository - Ensured that failures derived form correlating Document to its actual provenance event do fail the entire query and produce partial results with warning messages - Refactored DocsReader.read() operation. - Added test to validate two conditions where the such failures could occur
There was a problem hiding this comment.
Removing this public method is a breaking change. Recommend deprecating.
There was a problem hiding this comment.
Considering that this class does not appear to be intended as public API I still believe it is the right thing to do since the constructor argument isn't used.
In fact, need to look if we can make this class package private.
There was a problem hiding this comment.
Should consult the versioning guide to see if it addresses how to handle this. We need to be very explicit about which parts of the codebase are public and designed for extension versus those which the community should be free to evolve . if the guide doesn't cover this case let's solve it.
There was a problem hiding this comment.
I'm like 95% with @olegz on this. I believe it is implausible that someone would be calling this method and likely needed a comment that it was public for some other reason than it was a public API. That 5% not with @olegz is adding deprecation tag adds a tiny smidge of tech debt, which would be swept up when we investigate all the deprecated methods for 1.0.0
There was a problem hiding this comment.
We have designed flexibility in for a reason. Deprecation is appropriate for the public portions of code designed for extension and/or modification. For everything else let's be sure it is clear and documented what is fair game for the community to evolve. If it isn't clear then we should err on the side of deprecation then make it clear. We need to have agility behind the curtain.
There was a problem hiding this comment.
100% agree. Other projects use annotations to warn folks of risk when using public methods that aren't extension points (e.g. @PublicForTests or @experimental), but I'd settle for a comment.
There was a problem hiding this comment.
I agree that depreciation is the right thing to do in general cases like this. But given that the project is pre-1.0 and obvious intent of this class not being a public API I felt removing it would be ok. Still do but will honor the majority opinion and will deprecate.
There was a problem hiding this comment.
oleg i was entirely in agreement with how you had it for this case (removal). Tony was only 5% in disagreement. Of those that weighed in you have 195% support ;-)
There was a problem hiding this comment.
I think there has been a "handshake agreement" on the nature of the framework code versus the extension developer facing API. Having said that, I was not able to find any documentation that indicates that is the case. As a result, I think I am a similar 95/5 split as Tony. As a result, I think the "right" thing given the circumstances is to tag it up as Deprecated and remove at 1.x. However, I think it's likely a fair assumption that this is clear. Either way, we need to get the handshake agreement in a tangible form some place. I'll make a ticket and please add on/enhance as needed.
There was a problem hiding this comment.
@apiri The versioning scheme is defined at https://cwiki.apache.org/confluence/display/NIFI/Version+Scheme - it essentially says that all extension points that adhere to API version X.Y.* must also work in version X.(Y+1).*. I.e., minor changes will not break backward compatibility of extension point API or those objects that can be reached by extension points via the API.
|
Grabbed the PR locally and scoped it out. Nothing substantive to add beyond those comments here already. Performed some querying of a locally running instance with the new patch applied and didn't notice any tremendous performance impacts. Will continue to run as this patch goes through it's final paces. |
- made DocReader package private - polished logic in read(..) method to avoid escaping the loop - added call to sorting logic in LuceneUtil.groupDocsByStorageFileName(..) to ensure that previous behavior and assumptions in read(..) methodd are preserved - other minor polishing
|
@trkurc @joewitt @apiri
|
|
I'm mostly okay with package private. On the off chance someone was using this API it will be marginally more challenging to adapt to this change, but is possible. |
|
@trkurc Personally, I have exactly 0 qualms about changing it to package private. If I choose to take some random util class from a release of Apache Tomcat, for example, and depended on it, I should certainly not be surprised if that class changes from release to release (including incremental releases) - I don't think this is any different. |
This closes apache#123. Signed-off-by: Aldrin Piri <aldrin@apache.org>
6 participants
…nance repository