-
Notifications
You must be signed in to change notification settings - Fork 470
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
Added HTTP(S) connections to known limitations #11083
Conversation
Files changed: |
✔️ Netlify Preview 🔨 Explore the source changes: d463362 🔍 Inspect the deploy log: https://app.netlify.com/sites/cockroachdb-docs/deploys/611e75ee3595030007ba18c2 😎 Browse the preview: https://deploy-preview-11083--cockroachdb-docs.netlify.app/docs/v21.1/known-limitations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)
v21.1/known-limitations.md, line 170 at r1 (raw file):
As of v21.1, CockroachDB includes the [Cluster API](cluster-api.html), a REST API that accepts HTTP(S) requests for monitoring data. In a future release, we plan to add support for HTTP(S) proxies, such as [PostgREST](https://postgrest.org/en/v8.0/).
is this too definitive? Should we say "We may plan to add support for HTTP(s) proxies?
I went ahead and filed this as a more general issue cockroachdb/cockroach#69146 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR @awoods187 !
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @awoods187)
v21.1/known-limitations.md, line 170 at r1 (raw file):
Previously, awoods187 (Andy Woods) wrote…
is this too definitive? Should we say "We may plan to add support for HTTP(s) proxies?
Done.
@@ -161,6 +161,16 @@ UNION ALL SELECT * FROM t1 LEFT JOIN t2 ON st_contains(t1.geom, t2.geom) AND t2. | |||
|
|||
## Unresolved limitations | |||
|
|||
### HTTP(S) connections | |||
|
|||
CockroachDB does not support database connections across HTTP(S). All database connections must be made via TCP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this tech note is kind of confusing. IIUC, TCP is a transport layer protocol, HTTPS is an application layer protocol. they're different things? HTTP(S) can use TCP to communicate.
i wouldn't call this a "limitation", just more a thing we don't support (and it's not expected that we do?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CockroachDB does not support database connections across HTTP(S)
IIUC, this just means that CRDB doesn't include a built-in, fully-functional REST API. @awoods187 Is that correct?
All database connections must be made via TCP.
Would it make things a little clearer to remove this sentence?
Happy to make any other changes too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 we should try to clarify this a bit more.
Though also I don't think this should be under "limitations" but I may not understand what's being described here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my vote is to remove it entirely, cockroachdb/cockroach#69146 may contain part of the conversation.
Alright, I think I've been outvoted here :). Let's remove it from known limitations. Perhaps we can place something about no rest API connections on the connect to the database page? |
reverted here: #11828 |
Follow-up on https://cockroachlabs.slack.com/archives/CC5CX8EKE/p1629155750071600.
Please let me know if there is a better tracking issue to reference, as this limitation calls out our lack of support for HTTP(S) connections in general (and not just our incompatibility with PostgREST).