Skip to content

Extract common JDBC configurations into their own fragment#8643

Merged
hashhar merged 2 commits intotrinodb:masterfrom
hashhar:hashhar/jdbc-docs
Aug 4, 2021
Merged

Extract common JDBC configurations into their own fragment#8643
hashhar merged 2 commits intotrinodb:masterfrom
hashhar:hashhar/jdbc-docs

Conversation

@hashhar
Copy link
Copy Markdown
Member

@hashhar hashhar commented Jul 23, 2021

Split jdbc-type-mapping fragment into two. One for common JDBC connector configs and other for type mapping.

This will give a place to document common JDBC connector configurations as a follow-up.

@hashhar hashhar added the docs label Jul 23, 2021
@hashhar hashhar requested review from electrum and mosabua July 23, 2021 06:34
@cla-bot cla-bot bot added the cla-signed label Jul 23, 2021
@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Jul 23, 2021

There are two versions of wording for the non-transactional inserts - 2af3981 and 2b6c52d. Please advise which one should be kept.

@hashhar hashhar marked this pull request as draft July 23, 2021 06:40
@hashhar hashhar added the WIP label Jul 23, 2021
@findepi
Copy link
Copy Markdown
Member

findepi commented Jul 23, 2021

we shouldn't document things which have sensible defaults and generally don't need to be changed and exist only as a escape hatch.

i agree

@hashhar hashhar force-pushed the hashhar/jdbc-docs branch from 2b6c52d to cbbd5d3 Compare July 23, 2021 08:19
@mosabua
Copy link
Copy Markdown
Member

mosabua commented Jul 23, 2021

Fyi @jhlodin and @Jessie212

@hashhar hashhar marked this pull request as ready for review July 24, 2021 03:08
@hashhar hashhar removed the WIP label Jul 24, 2021
@jhlodin jhlodin requested review from Ordinant, m57lyra and rosewms July 29, 2021 15:09
@jhlodin
Copy link
Copy Markdown
Contributor

jhlodin commented Jul 30, 2021

FYI: At @electrum 's request, Cherry-picked the JDBC type mapping fragments commit into its own PR here - #8727

Copy link
Copy Markdown
Contributor

@rosewms rosewms left a comment

Choose a reason for hiding this comment

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

I agree with the feedback from Joe and Annie.

@hashhar hashhar force-pushed the hashhar/jdbc-docs branch from cbbd5d3 to 278174f Compare August 2, 2021 15:33
@hashhar hashhar changed the title Document additional JDBC connector configurations Extract common JDBC configurations into their own fragment Aug 2, 2021
@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Aug 2, 2021

@jhlodin @electrum I've simplified this to just be a refactor that splits 1 file into two. I hope this is not controversial.

The new common JDBC configs (from BaseJdbcConfig etc.) can now start getting documented under jdbc-common-configurations fragment.

@hashhar hashhar force-pushed the hashhar/jdbc-docs branch from 278174f to dee6436 Compare August 2, 2021 15:39
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.

Mostly LGTM, but the Phoenix connector doc is confusing. Left a comment with suggestions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CREATE TABLE

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Pre-existing. Noted down, will fix in a later change. I'd like to keep this PR isolated to just mechanical splitting of things without changing unrelated things.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rephrase to remove "are available" it sounds awkward. I'd recommend something like:

Review the available Phoenix-specific configuration properties:

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.

Works for me to split it up.

@hashhar hashhar force-pushed the hashhar/jdbc-docs branch from 8978a0c to 674dda1 Compare August 3, 2021 18:20
@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Aug 3, 2021

Squashed and merged.

@hashhar hashhar added this to the 361 milestone Aug 3, 2021
Copy link
Copy Markdown
Contributor

@rosewms rosewms left a comment

Choose a reason for hiding this comment

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

LGTM

@hashhar hashhar merged commit 4394ce9 into trinodb:master Aug 4, 2021
@hashhar hashhar deleted the hashhar/jdbc-docs branch August 4, 2021 05:49
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.

7 participants