Skip to content

Conversation

ChillFish8
Copy link

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.

Copy link

ti-chi-bot bot commented Aug 31, 2025

Welcome @ChillFish8! It looks like this is your first PR to tikv/fail-rs 🎉

@ChillFish8 ChillFish8 force-pushed the chillfish8/add-crate-isolation branch from 8c1a7d1 to 566cde5 Compare August 31, 2025 02:16
Copy link

ti-chi-bot bot commented Aug 31, 2025

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:

  • 566cde5 Add crate isolation of fail points

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

@ChillFish8
Copy link
Author

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

@BusyJay
Copy link
Member

BusyJay commented Sep 1, 2025

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 foo::bar::failpiotn.

@ChillFish8
Copy link
Author

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 parking_lot's send_guard and deadlock_detection features. Ultimately, we can only warn the developer not to enable the feature outside of their developer dependencies.

@ChillFish8
Copy link
Author

I was thinking some more about this issue, and maybe a good compromise is doing something similar to tokio_unstable and having this configuration be part of a conditional compilation config variable rather than a feature.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants