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

Update to the JDBCStore section of the Transaction guide #36179

Merged

Conversation

MichalMaler
Copy link
Contributor

@MichalMaler MichalMaler commented Sep 27, 2023

Adding:

  • Some information mentioned by @maxandersen in the Transaction gdoc draft.
  • Link to a complete list of Narayana configuration options.
  • Additional context info and proper terminology.
  • Removing metadata :sumary: from the top of the file since this is automatically taken from the first sentence that acts as guide intro.
  • Adding metadata diataxis-template: reference
  • Tiny style tweaks.
  • Proper headings capitalization

@zhfeng @mmusgrov , please have a look and provide an SME review when time and mood.
Thank you.

@github-actions
Copy link

github-actions bot commented Sep 27, 2023

🙈 The PR is closed and the preview is expired.

@MichalMaler MichalMaler requested review from michelle-purcell and maxandersen and removed request for michelle-purcell September 29, 2023 11:13
@MichalMaler MichalMaler added the area/docstyle issues related for manual docstyle review label Oct 2, 2023
@MichalMaler MichalMaler force-pushed the Transaction-Storing-to-a-database-update branch from fe8e686 to 89cf973 Compare October 2, 2023 10:59
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
Quarkus Documentation automation moved this from To do to Review pending Oct 2, 2023
@MichalMaler MichalMaler force-pushed the Transaction-Storing-to-a-database-update branch from 89cf973 to b4469cc Compare October 2, 2023 12:11
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
@MichalMaler
Copy link
Contributor Author

Removing this bit:

Storing transaction logs into a database is slower, creates bigger overhead, and requires higher maintenance.
Use this approach when prioritizing consistency over throughput.

@MichalMaler MichalMaler force-pushed the Transaction-Storing-to-a-database-update branch from b4469cc to a5d654f Compare October 3, 2023 13:48
@MichalMaler
Copy link
Contributor Author

@mmusgrov @jmfinelli
Thank you for your feedback. I applied it to my best extend. Pleas, have a look on it now:
https://github.com/quarkusio/quarkus/blob/a5d654f859ae23f7524e2f95494d274178a5c7b2/docs/src/main/asciidoc/transaction.adoc#configure-storing-of-quarkus-transaction-logs-in-a-database

If you have any additional suggestions, please do one of the following so that we are not creating the comment wall of text again:

  1. Apply the direct suggestion which I can accept and commit
    or
  2. Have a look on the entire text in this doc and lets tune it to your ideal shape there. https://docs.google.com/document/d/1jLNPsgC57V3S1FjWpbWJJcRB13-MSi_WmVneABOitMc/edit

docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
@mmusgrov
Copy link
Contributor

mmusgrov commented Oct 4, 2023

@mmusgrov @jmfinelli Thank you for your feedback. I applied it to my best extend. Pleas, have a look on it now: https://github.com/quarkusio/quarkus/blob/a5d654f859ae23f7524e2f95494d274178a5c7b2/docs/src/main/asciidoc/transaction.adoc#configure-storing-of-quarkus-transaction-logs-in-a-database

If you have any additional suggestions, please do one of the following so that we are not creating the comment wall of text again:

  1. Apply the direct suggestion which I can accept and commit

That could be a lot of work for one of us. @jmfinelli , should one of us rewrite the quarkus transaction guide, unfortunately the transactions team are under resourced and we have a backlog of work.

or
2. Have a look on the entire text in this doc and lets tune it to your ideal shape there. https://docs.google.com/document/d/1jLNPsgC57V3S1FjWpbWJJcRB13-MSi_WmVneABOitMc/edit

I don't really understand that document, how does it relate to this PR?

@MichalMaler
Copy link
Contributor Author

@mmusgrov @jmfinelli Thank you for your feedback. I applied it to my best extend. Pleas, have a look on it now: https://github.com/quarkusio/quarkus/blob/a5d654f859ae23f7524e2f95494d274178a5c7b2/docs/src/main/asciidoc/transaction.adoc#configure-storing-of-quarkus-transaction-logs-in-a-database
If you have any additional suggestions, please do one of the following so that we are not creating the comment wall of text again:

  1. Apply the direct suggestion which I can accept and commit

That could be a lot of work for one of us. @jmfinelli , should one of us rewrite the quarkus transaction guide, unfortunately the transactions team are under resourced and we have a backlog of work.

or
2. Have a look on the entire text in this doc and lets tune it to your ideal shape there. https://docs.google.com/document/d/1jLNPsgC57V3S1FjWpbWJJcRB13-MSi_WmVneABOitMc/edit

I don't really understand that document, how does it relate to this PR?

The content of this PR can be find on the page 1 and 2 of the attached file.

I will go trough your suggestion one more time and rewrite start of this chapter, even if it was not target of this PR.
Then, I would kindly asked a Transaction guide SME for merge or close.
Any member of the Narayana team then can open a new PR and add additional details.

@MichalMaler MichalMaler force-pushed the Transaction-Storing-to-a-database-update branch from a5d654f to a53998c Compare October 5, 2023 09:08
@MichalMaler
Copy link
Contributor Author

@mmusgrov @jmfinelli
Can you let me know if this Solved issue was already implemented to Quarkus?
https://issues.redhat.com/browse/AG-209

If so, I can remove this part from the guide.

[NOTE]
====
To work around the current known issue of link:https://issues.redhat.com/browse/AG-209[Agroal having a different view on running transaction checks], set the datasource transaction type for the datasource responsible for writing the transaction logs to `disabled`:

----
quarkus.datasource.TX_LOG.jdbc.transactions=disabled
----

This example uses TX_LOG as the datasource name.
====

@MichalMaler MichalMaler force-pushed the Transaction-Storing-to-a-database-update branch from a53998c to aba9bdb Compare October 5, 2023 09:23
Copy link
Contributor

@zhfeng zhfeng left a comment

Choose a reason for hiding this comment

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

LGTM

@MichalMaler MichalMaler force-pushed the Transaction-Storing-to-a-database-update branch from 416dcb6 to 7385343 Compare October 16, 2023 10:55
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Show resolved Hide resolved
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

Here's a review as you requested, @MichalMaler ... Not sure this helps much, sorry :/

docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/transaction.adoc Show resolved Hide resolved
@MichalMaler MichalMaler force-pushed the Transaction-Storing-to-a-database-update branch from c18dfe4 to e8019fd Compare October 16, 2023 12:17
Copy link
Member

@yrodiere yrodiere 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 to me, thanks.

@jmfinelli Can you confirm your concerns were addressed? Thanks.

Quarkus Documentation automation moved this from Review pending to Reviewer approved Oct 16, 2023
@jmfinelli
Copy link

jmfinelli commented Oct 16, 2023

Looks good to me, thanks.

@jmfinelli Can you confirm your concerns were addressed? Thanks.

Sorry @yrodiere. I am trying review this PR as soon as possible but, as the git history is re-written all the time, is not easy.

Copy link

@jmfinelli jmfinelli left a comment

Choose a reason for hiding this comment

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

Thanks @MichalMaler. Just one last modification 👍

docs/src/main/asciidoc/transaction.adoc Outdated Show resolved Hide resolved
Copy link

@jmfinelli jmfinelli left a comment

Choose a reason for hiding this comment

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

Thanks @MichalMaler :-)

Apply suggestions from code review

Co-authored-by: Yoann Rodière <yoann@hibernate.org>

Removing redundant contant

Applying jmfinelli's suggestions for a better wording

Signed-off-by: Michal Maléř <mmaler@redhat.com>
@MichalMaler MichalMaler force-pushed the Transaction-Storing-to-a-database-update branch from cad45a3 to 6dabad6 Compare October 16, 2023 14:16
@MichalMaler MichalMaler requested review from mmusgrov and jmartisk and removed request for mmusgrov October 16, 2023 15:57
@Ladicek Ladicek merged commit 0a8efce into quarkusio:main Oct 17, 2023
5 checks passed
Quarkus Documentation automation moved this from Reviewer approved to Done Oct 17, 2023
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Oct 17, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 17, 2023
@MichalMaler MichalMaler deleted the Transaction-Storing-to-a-database-update branch October 17, 2023 09:24
@aloubyansky aloubyansky modified the milestones: 3.6 - main, 3.2.8.Final Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docstyle issues related for manual docstyle review area/documentation
Development

Successfully merging this pull request may close these issues.

None yet

9 participants