Conversation
|
Welcome @ChillFish8! It looks like this is your first PR to tikv/fail-rs 🎉 |
8c1a7d1 to
566cde5
Compare
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@BusyJay Let me know what I need to do re any changes and this DCO bot, I am not entirely sure what it wants from me or In what format. Thanks, Harrison. |
|
I don't think using feature to do this is a good idea. If a library in the dependency tree enables the feature, it will be enabled for all dependents. Why not just add namespace to failpoint definition? For example name the failpoint as |
|
We do this currently; however, it leaves something to be desired on the consistency side of things and is out of our control on the libraries that we don't maintain if they expose fail points of their own. I agree that the feature solution is imperfect, but it is probably the best option because we want to at least try not to break existing implementations that don't want this change. I think overall the feature itself is not that different to what other crates have done and do with mutually exclusive features, like |
|
I was thinking some more about this issue, and maybe a good compromise is doing something similar to That way, the only one able to change the behaviour is the end user binary/test, and it removes the possibility for any issues with libraries adding the feature that then enables it for everything in the tree while still providing the option of this forced namespacing. |
In reference to #69
Firstly, thank you for this crate, it is incredibly useful. For context, what I am currently doing is trying to put a set of fail points behind feature flags within my crates to allow for easier testing of downstream libraries that use it.
This PR is similar to the idea mentioned in #69 but does not make it the default, instead it makes this an opt-in decision for the end user to prevent this being a breaking change of behaviour (as confirmed by testing.)
I've tested this more verbosely by creating a chain of crates that depend on fail points from within each other; however, it is quite bulky, so I'm not sure if you'd like me to include them in this PR or continue to leave them out.