Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document query.reuse-connection configuration property #14870

Closed

Conversation

colebow
Copy link
Member

@colebow colebow commented Nov 2, 2022

Description

Docs for #14653 - added to JDBC fragment to propagate to all connector pages.

Non-technical explanation

Docs only.

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@@ -51,3 +51,7 @@ connector:
Using a large timeout can potentially result in more detailed dynamic filters.
However, it can also increase latency for some queries.
- ``20s``
* - ``query.reuse-connection``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a kill switch, added only in case of bugs. I would prefer not document this and possibly remove in future once we see there are no issues related to this new feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added to the release notes...

Copy link
Member Author

@colebow colebow Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that if users run into issues, ideally they should be able to figure out how to work around it by looking at connector docs, not by looking at release notes.

If/when we decide to remove the property, it should be easy to also remove the docs at that juncture, yeah?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we document all the kill switches.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how are users supposed to know about it, if we don't document or expose it? I think we should document them. And if we want to remove them at some stage .. we can.

Or are you saying you want to prevent users from knowing about it and/or using them. If so .. then why did we add it in the first place. Purely for developers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is here, so in case we hear a feedback that it does not work then we may want to suggest to use it. I plan to remove the kill switch after some time if there is no feedback.

I we place this in documentation then we may not hear the feedback at all and we could just have bugs that people bypass while I would like to fix these bugs.

Copy link
Member

@hashhar hashhar Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem with documenting kill switches is that users enable those to avoid change (I know I did) and then you can never know if people actually use the new code and then one day you remove kill switch and people start filing bugs that thing was broken.

It's tricky to balance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here is what I would do. Always do one or the other:

  • implement a kill switch and document it
  • don't implement a kill switch

If you need to later remove a kill switch .. you could leave it behind as a no-op with a warning for a while and then remove it.

@mosabua
Copy link
Member

mosabua commented Nov 22, 2022

So at this stage it looks like this wont get merged right @hashhar @kokosing @findepi ?

If so .. lets close the PR @colebow

@colebow colebow closed this Nov 29, 2022
@colebow colebow deleted the colebow/reuse-connection-docs branch January 13, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants