Skip to content

Document HTTP client properties#12247

Merged
electrum merged 1 commit intotrinodb:masterfrom
Jessie212:jt/airlift
Jul 29, 2022
Merged

Document HTTP client properties#12247
electrum merged 1 commit intotrinodb:masterfrom
Jessie212:jt/airlift

Conversation

@Jessie212
Copy link
Copy Markdown
Contributor

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Documentation only.

How would you describe this change to a non-technical end user or system administrator?

Document HTTP client OAuth prefix

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 4, 2022
@Jessie212 Jessie212 requested a review from mosabua May 4, 2022 19:11
@Jessie212
Copy link
Copy Markdown
Contributor Author

Jessie212 commented May 4, 2022

First draft outline for page. Is this what we want?

@github-actions github-actions bot added the docs label May 4, 2022
@Jessie212 Jessie212 changed the title Document HTTP client OAuth prefix Document HTTP client properties May 9, 2022
@mosabua
Copy link
Copy Markdown
Member

mosabua commented May 10, 2022

@Jessie212 .. please work with @bowdencm to get all the technical details to a level where you are both happy .. and then I can review. He can clarify most if not all of this.

@electrum
Copy link
Copy Markdown
Member

  • failure-detector
  • workerInfo
  • memoryManager
  • node-manager
  • exchange
  • jwk (JWT authenticator)

@Jessie212
Copy link
Copy Markdown
Contributor Author

Jessie212 commented Jun 1, 2022

@lukasz-walkiewicz Please take a look at the remaining property descriptions. The ones with question marks are my best attempts at describing the properties.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure here but IIRC it's platform and configuration dependent. @electrum wdyt?

Copy link
Copy Markdown
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

We might want to split these into sections:

  • General
  • TLS
  • Proxy

@Jessie212
Copy link
Copy Markdown
Contributor Author

@mosabua @jhlodin Did my best to group the properties. Please take a look.

Copy link
Copy Markdown
Contributor

@jhlodin jhlodin left a comment

Choose a reason for hiding this comment

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

Some minor rewordings needed, otherwise LGTM

@Jessie212 Jessie212 force-pushed the jt/airlift branch 2 times, most recently from ad183a0 to a87857f Compare June 17, 2022 15:55
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks great now

@colebow colebow requested a review from electrum June 22, 2022 19:16
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

This looks good now. Can we get this merged @electrum ?

@Jessie212
Copy link
Copy Markdown
Contributor Author

@electrum I've applied your suggestions. It's ready to merge.

Copy link
Copy Markdown
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Apologies for the long delay. Generally looks good, but needs a few changes

@Jessie212
Copy link
Copy Markdown
Contributor Author

@electrum Thanks for the comments and suggestions, David. I've addressed them all. This PR is ready for merging.

@electrum electrum merged commit 2b26212 into trinodb:master Jul 29, 2022
@github-actions github-actions bot added this to the 392 milestone Jul 29, 2022
@Jessie212 Jessie212 deleted the jt/airlift branch August 29, 2022 13:48
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.

6 participants