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

file system cache doc change #22176

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MehulBatra
Copy link

@MehulBatra MehulBatra commented May 28, 2024

Description

File system cache document change on optional fields

Additional context and related issues

As a developer when we try to enable caching, we face errors for setting options and try to look through it via logs what went wrong, if we could specify proactively that would make things easier.

Release notes

(*) This is not user-visible or is 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:

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

Copy link

cla-bot bot commented May 28, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link
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.

Not sure this improves much and need to figure out if or how we highlight optional vs required. The issue is that we do NOT do that anywhere else in the docs and it really should stay consistent. Also the braces are definitely against the Google doc style we use, italics might be fine but still seems against the rest of the docs and overkill.

If you adjust to the comments and format to 80 char column width we can merge after another review.

docs/src/main/sphinx/object-storage/file-system-cache.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/object-storage/file-system-cache.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/object-storage/file-system-cache.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/object-storage/file-system-cache.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/object-storage/file-system-cache.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/object-storage/file-system-cache.md Outdated Show resolved Hide resolved
@mosabua
Copy link
Member

mosabua commented May 28, 2024

Also update commit message to follow https://cbea.ms/git-commit/

@mosabua
Copy link
Member

mosabua commented May 28, 2024

I chatted with some other writers on the team - for now we do not want to use () or italics for the required and optional terms since it does not provide much additional benefit but introduces an inconsistency with all the other docs.

@MehulBatra
Copy link
Author

MehulBatra commented May 28, 2024

I chatted with some other writers on the team - for now we do not want to use () or italics for the required and optional terms since it does not provide much additional benefit but introduces an inconsistency with all the other docs.

@mosabua I picked up the idea of () from referring to the Trino Kubernetes docs itself, I found it quite clear as it was explicitly marked optional and caught my attention.
image

Copy link

cla-bot bot commented May 28, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

3 similar comments
Copy link

cla-bot bot commented May 28, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented May 28, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented May 28, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@mosabua
Copy link
Member

mosabua commented May 28, 2024

I chatted with some other writers on the team - for now we do not want to use () or italics for the required and optional terms since it does not provide much additional benefit but introduces an inconsistency with all the other docs.

@mosabua I picked up the idea of () from referring to the Trino Kubernetes docs itself, I found it quite clear as it was explicitly marked optional and caught my attention. image

Hm .. interesting .. I am not sure but to keep it simple I would suggest not to do that for now .. we can revisit for the complete docs or maybe take it out in the k8s docs.

@mosabua
Copy link
Member

mosabua commented May 28, 2024

Also please squash commits and update commit message and submit CLA.

@MehulBatra
Copy link
Author

Also please squash commits and update commit message and submit CLA.

CLA I have already submitted via email, changing the commit message and squashing it!

Copy link

cla-bot bot commented May 28, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented May 28, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@mosabua
Copy link
Member

mosabua commented May 28, 2024

Please fix wrapping to 80 character columns while we wait for CLA processing

Copy link

cla-bot bot commented May 29, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link
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 good now. Just waiting for CLA processing to be done by @martint

@MehulBatra
Copy link
Author

@mosabua @martint when can we expect the cla to be processed, thank you!
Please do let me know if anything is missing from my end.

Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jun 24, 2024
@mosabua
Copy link
Member

mosabua commented Jun 24, 2024

@cla-bot check

Copy link

cla-bot bot commented Jun 24, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented Jun 24, 2024

The cla-bot has been summoned, and re-checked this pull request!

@mosabua
Copy link
Member

mosabua commented Jun 24, 2024

I just recently fixed something else in this section so you will have to rebse @MehulBatra .. sorry. CLA processing will happen soon.

@github-actions github-actions bot removed the stale label Jun 25, 2024
@mosabua
Copy link
Member

mosabua commented Jun 26, 2024

@cla-bot check

Copy link

cla-bot bot commented Jun 26, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented Jun 26, 2024

The cla-bot has been summoned, and re-checked this pull request!

@mosabua
Copy link
Member

mosabua commented Jun 26, 2024

@MehulBatra CLA was processed now, seems like your git or github config is not using the right email and CLA bot verification is not working, also please rebase and resolve conflict.

Once all is good please ping me and I will review again and merge.

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

Successfully merging this pull request may close these issues.

None yet

3 participants