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

Add a extra note for jdbc object store configuration #34297

Closed
wants to merge 1 commit into from

Conversation

zhfeng
Copy link
Contributor

@zhfeng zhfeng commented Jun 26, 2023

It's related to #33739

@github-actions
Copy link

github-actions bot commented Jun 26, 2023

🙈 The PR is closed and the preview is expired.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jun 26, 2023

Hello @zhfeng ! Good job on putting this together. I created a review for this little section and want to share it here with you. Please, use it if you will see it fits well. It is rewritten to match our current docs guidelines.

[[jdbcstore]]
== Configure the transaction log to be stored in a datasource

JDBCStore is a class in the Narayana transaction management library that facilitates the storage of transaction logs in a database by using JDBC.

JDBCStore is our recommended approach for users needing transaction recovery capabilities, especially when running in volatile containers.

To enable this capability, explicitly set `quarkus.transaction-manager.object-store.type` to `jdbc`.
You can also specify a datasource name for the transaction log storage by setting `quarkus.transaction-manager.object-store.datasource`.
It will use the default datasource configuration if not specified.

When the `quarkus.transaction-manager.object-store.create-table` configuration is enabled, the transaction log table is automatically created if it does not already exist.
However, to enable this capability, the transaction node identifier must be set through `quarkus.transaction-manager.node-name`.

NOTE: The recommended workaround for the known issue link:https://issues.redhat.com/browse/AG-209[AG-209] is to set the datasource transaction type to `disabled`.

@zhfeng zhfeng force-pushed the issue_33739_doc branch from 8b02998 to ff760a6 Compare June 26, 2023 11:46
@zhfeng
Copy link
Contributor Author

zhfeng commented Jun 26, 2023

@mmusgrov @yrodiere can you take a review of these document changes?

Copy link
Contributor

@mmusgrov mmusgrov left a comment

Choose a reason for hiding this comment

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

Nice work.
I did have two comments mind.


To enable this capability, you need to set `quarkus.transaction-manager.object-store.type` to `jdbc` explicitly. Also, you can specify a datasource name to be used for the transaction log storage by setting `quarkus.transaction-manager.object-store.datasource`. It will use the default datasource configuration if not specified.
JDBCStore is our recommended approach for users needing transaction recovery capabilities, especially when running in volatile containers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the recommendation come from? I can understand why it might be easier to use in a cloud environment but the file or journal stores are normally recommended. Even volatile containers are normally configured to use persistent volumes.

The reason I've picked on this statement is that the JDBCStore is a lot slower than the journal or file system and performance of transactions is one of the primary concerns with using transactions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I worked with the original text where the recommendation was already applied. Let me draft a second version of this whole section so that I can help @zhfeng with this a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mmusgrov Whats are the benefits of this feature then, please?

Copy link
Member

Choose a reason for hiding this comment

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

Can't comment on the recommendation (@mmusgrov knows best), but I suspect persistent volumes cannot always (efficiently) be shared across container instances, and then there's the problem of handling "dangling" volumes once a container died for some reason (scaling down, ...) and there are some uncommitted transactions in the transaction log.

Anyway, I'm just guessing, and -- as is probably apparent -- I'm not fully aware of the problematic we're tackling here. Ideally we'd just ask whoever requested this feature be added to Quarkus; they must have a pretty good idea of the exact intended use case. I don't know who that is, though.

Regardless, like above, I'd recommend against using the name of the JDBCStore class in the documentation. Maybe something like this:

Database storage of transaction logs generally performs worse than journal of filesystem storage, but may be necessary in cloud environment lacking persistent storage (for example if your application container cannot use persistent volumes).

Or something similar.

Copy link
Contributor

@mmusgrov mmusgrov Jun 27, 2023

Choose a reason for hiding this comment

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

@mmusgrov Whats are the benefits of this feature then, please?

Yoann suggested some nice text earlier that you can use here (Database storage of transaction logs generally performs worse than journal or filesystem storage, but may be necessary in cloud environments lacking persistent storage (for example if your application container cannot use persistent volumes).


NOTE: When enabling this capability, the transaction node identifier must be set through `quarkus.transaction-manager.node-name`.
When the `quarkus.transaction-manager.object-store.create-table` configuration is enabled, the transaction log table is automatically created if it does not already exist.
However, to enable this capability, the transaction node identifier must be set through `quarkus.transaction-manager.node-name`.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the nodeId isn't set then a default value is used so this sentence is not clear - ie the transaction manager always has a nodeId (and it gets embedded in XIDs). Perhaps you are referring to requirement for a unique nodeId but if you are then the same statement will apply to any object store type and is not specific to the JDBCStore so it looks out of place to draw attention to it here.

@@ -362,16 +362,21 @@ NOTE: The `event` object represents the transaction ID, and defines `toString()`
TIP: In listener methods, you can access more information about the transaction in progress by accessing the `TransactionManager`,
which is a CDI bean and can be ``@Inject``ed.

== Configuring transaction log to be stored in a DataSource
[[jdbcstore]]
== Configure the transaction log to be stored in a dataSource
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you call it a dataSource instead of a database. I ask because a dataSource can be backed by other storage such as a filesystem, for example, and in this case we'd recommend that users use the file or journal stores directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its because Narayana is mention in a Datasource guide section, so calling it like that creates the logical follow-up.
The usage of the database vs datasource (or even data source) is a little problem overall all our docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

But a datasource is a factory for creating connections to the physical data source, ie a transaction log is not stored in a dataSource/factory. If you want to stay with this terminology, then use the generic term data source. But see my other comment stating that data sources may also refer to storage backed by a filesystem.


The Narayana project has the capability to store the transaction logs into a JDBC Datasource; this should be our recommendation for users needing transaction recovery capabilities, especially when running in volatile containers.
JDBCStore is a class in the Narayana transaction management library that facilitates the storage of transaction logs in a database by using JDBC.
Copy link
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 the name of a class inside Narayana is relevant to Quarkus users, since they will never manipulate it.

Maybe:

Suggested change
JDBCStore is a class in the Narayana transaction management library that facilitates the storage of transaction logs in a database by using JDBC.
Transaction management may be configured to store transaction logs into a database accessed through JDBC.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1


To enable this capability, you need to set `quarkus.transaction-manager.object-store.type` to `jdbc` explicitly. Also, you can specify a datasource name to be used for the transaction log storage by setting `quarkus.transaction-manager.object-store.datasource`. It will use the default datasource configuration if not specified.
JDBCStore is our recommended approach for users needing transaction recovery capabilities, especially when running in volatile containers.
Copy link
Member

Choose a reason for hiding this comment

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

Can't comment on the recommendation (@mmusgrov knows best), but I suspect persistent volumes cannot always (efficiently) be shared across container instances, and then there's the problem of handling "dangling" volumes once a container died for some reason (scaling down, ...) and there are some uncommitted transactions in the transaction log.

Anyway, I'm just guessing, and -- as is probably apparent -- I'm not fully aware of the problematic we're tackling here. Ideally we'd just ask whoever requested this feature be added to Quarkus; they must have a pretty good idea of the exact intended use case. I don't know who that is, though.

Regardless, like above, I'd recommend against using the name of the JDBCStore class in the documentation. Maybe something like this:

Database storage of transaction logs generally performs worse than journal of filesystem storage, but may be necessary in cloud environment lacking persistent storage (for example if your application container cannot use persistent volumes).

Or something similar.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jun 26, 2023

@mmusgrov @yrodiere
OK, here is my revamp.
I will ask @xstefank to have a look too.

[[jdbcstore]]
== Support for storing Quarkus transaction logs using a datasource

`JDBCStore` is a class in the Narayana transaction management library that facilitates the storage of transaction logs in a database by using a JDBC datasource.

JDBCStore is an alternative to the file or journal stores transaction recovery, suitable for use in a cloud environment or when running in volatile containers without the possibility of using persistent volumes.

To enable this capability, explicitly set `quarkus.transaction-manager.object-store.type` to `jdbc`.
You can also specify the datasource name for the transaction log storage by setting `quarkus.transaction-manager.object-store.datasource`.
If no configuration is provided, Quarkus uses the default datasource. 

When the `quarkus.transaction-manager.object-store.create-table` configuration is enabled, the transaction log table gets automatically created if it does not already exist.

IMPORTANT: To work around the known issue link:https://issues.redhat.com/browse/AG-209[AG-209], set the datasource transaction type to `disabled`.

@mmusgrov
Copy link
Contributor

@mmusgrov @yrodiere OK, here is my revamp. I will ask @xstefank to have a look too.

I think Martin's expertise is with LRA rather than the transaction log store types.

[[jdbcstore]]
== Support for storing Quarkus transaction logs using a datasource

See my earlier comment about using the term datasource.

JDBCStore is a class in the Narayana transaction management library that facilitates the storage of transaction logs in a database by using a JDBC datasource.

Yoann had a comment and suggestion about using the term JDBCStore.
He also had a nice suggestion for some text about the relative performance of the various stores.

JDBCStore is an alternative to the file or journal stores transaction recovery, suitable for use in a cloud environment or when running in volatile containers without the possibility of using persistent volumes.

To enable this capability, explicitly set quarkus.transaction-manager.object-store.type to jdbc.
You can also specify the datasource name for the transaction log storage by setting quarkus.transaction-manager.object-store.datasource.
If no configuration is provided, Quarkus uses the default datasource.

When the quarkus.transaction-manager.object-store.create-table configuration is enabled, the transaction log table gets automatically created if it does not already exist.

IMPORTANT: To work around the known issue link:https://issues.redhat.com/browse/AG-209[AG-209], set the datasource transaction type to disabled.

@xstefank
Copy link
Member

@mmusgrov is correct, but I agree with the suggestions reviewers made. Otherwise, it makes sense to me.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jun 27, 2023

@zhfeng Here is the reworked version that containts @mmusgrov and @yrodiere suggestions.

[[jdbcstore]]
== Support for storing Quarkus transaction logs in a database

Transaction management can be configured to store transaction logs in a database by using a JDBC datasource.

Database storage is an alternative to the file or journal stores transaction recovery, suitable for use in a cloud environment lacking persistent storage.
A use case example for database storage may be an application, the containers of which can not use persistent volumes.
However, database storage of transaction logs generally performs worse than a journal or filesystem options.

To enable this capability, explicitly set `quarkus.transaction-manager.object-store.type` to `jdbc`.

You can also specify the datasource name for the transaction log storage by setting `quarkus.transaction-manager.object-store.datasource`.
If no configuration is provided, Quarkus uses the default datasource. 

When the `quarkus.transaction-manager.object-store.create-table` configuration is enabled, the transaction log table gets automatically created if it does not already exist.

IMPORTANT: To work around the known issue of Agroal having a different view on running transaction checks, set the datasource transaction type to `disabled`.

@geoand
Copy link
Contributor

geoand commented Jun 27, 2023

Is this meant to close #33739? If so, please update the description so that issue gets closed when this gets merged.

Thanks

@zhfeng
Copy link
Contributor Author

zhfeng commented Jun 27, 2023

Thanks all for the inputs and I will update the PR with this new version.

@geoand No, it will not close #33739 and we only update the document to add a work around with AG-209.

@zhfeng zhfeng force-pushed the issue_33739_doc branch from ff760a6 to 6494ac0 Compare June 27, 2023 11:48
@mmusgrov
Copy link
Contributor

Database storage is an alternative to the file or journal stores transaction recovery, suitable for use in a cloud environment lacking persistent storage.

I don't think the narayana-jta extension supports the journal based store, is that correct @zhfeng ?

A use case example for database storage may be an application, the containers of which can not use persistent volumes.
However, database storage of transaction logs generally performs worse than a journal or filesystem options.

The grammar is not correct - remove the word "a" and the text does not read well, my suggestion is to replace these three sentences with Yoann's text, something like:

Database storage of transaction logs generally performs worse than filesystem storage, but may be necessary in cloud environments lacking persistent storage (for example if your application container cannot use persistent volumes).

To enable this capability, explicitly set quarkus.transaction-manager.object-store.type to jdbc.

@zhfeng is the default still FileSystem?
Do we need to document that anywhere?

You can also specify the datasource name for the transaction log storage by setting quarkus.transaction-manager.object-store.datasource.
If no configuration is provided, Quarkus uses the default datasource.

Is it documented elsewhere what the default is?

When the quarkus.transaction-manager.object-store.create-table configuration is enabled, the transaction log table gets automatically created if it does not already exist.

IMPORTANT: To work around the known issue of Agroal having a different view on running transaction checks, set the datasource transaction type to disabled.

Does the quarkus documentation style guide provide a recommendation for highlighting important text? For example the EAP docs use special fonts and background for this purpose.

Copy link
Contributor

@mmusgrov mmusgrov left a comment

Choose a reason for hiding this comment

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

I added a few change requests in my last comment

@zhfeng
Copy link
Contributor Author

zhfeng commented Jun 28, 2023

Database storage is an alternative to the file or journal stores transaction recovery, suitable for use in a cloud environment lacking persistent storage.

I don't think the narayana-jta extension supports the journal based store, is that correct @zhfeng ?

Yes, the journal based store is not supported in quarkus right now. Only FileSystem and JDBC is supported.

A use case example for database storage may be an application, the containers of which can not use persistent volumes.
However, database storage of transaction logs generally performs worse than a journal or filesystem options.

The grammar is not correct - remove the word "a" and the text does not read well, my suggestion is to replace these three sentences with Yoann's text, something like:

Database storage of transaction logs generally performs worse than filesystem storage, but may be necessary in cloud environments lacking persistent storage (for example if your application container cannot use persistent volumes).

To enable this capability, explicitly set quarkus.transaction-manager.object-store.type to jdbc.

@zhfeng is the default still FileSystem? Do we need to document that anywhere?

Yeah, it is.

You can also specify the datasource name for the transaction log storage by setting quarkus.transaction-manager.object-store.datasource.
If no configuration is provided, Quarkus uses the default datasource.

Is it documented elsewhere what the default is?

I think it could be https://quarkus.io/guides/datasource#default-datasource

When the quarkus.transaction-manager.object-store.create-table configuration is enabled, the transaction log table gets automatically created if it does not already exist.
IMPORTANT: To work around the known issue of Agroal having a different view on running transaction checks, set the datasource transaction type to disabled.

Does the quarkus documentation style guide provide a recommendation for highlighting important text? For example the EAP docs use special fonts and background for this purpose.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jun 28, 2023

@zhfeng , adding one more revision when using @yrodiere exact wording without the bracket usage.

I added the xref link you pointed out regarding the default (unnamed) datasource. These changes are already part of the main docs snapshot https://quarkus.io/version/main/guides/datasource#dev-services for few weeks now. Not sure why it is not part of the latest build yet.

[[jdbcstore]]
== Support for storing Quarkus transaction logs in a database

Transaction management can be configured to store transaction logs in a database by using a JDBC datasource.

Database storage of transaction logs generally performs worse than filesystem storage, but may be necessary in cloud environments lacking persistent storage, for example, if your application container cannot use persistent volumes.

To enable this capability, explicitly set `quarkus.transaction-manager.object-store.type` to `jdbc`.

You can also specify the datasource name for the transaction log storage by setting `quarkus.transaction-manager.object-store.datasource`.
If no configuration is provided, Quarkus uses the xref:datasource.adoc#dev-services[default datasource].

When the `quarkus.transaction-manager.object-store.create-table` configuration is enabled, the transaction log table gets automatically created if it does not already exist.

To work around the known issue of Agroal having a different view on running transaction checks, set the datasource transaction type to `disabled`.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jun 28, 2023

@mmusgrov We highlight exact names (properties, values, dependencies..) or parts of code with backticks.
Parts of GUI should be highlighted in Bold.

@mmusgrov
Copy link
Contributor

@mmusgrov We highlight exact names (properties, values, dependencies..) or parts of code with backticks. Parts of GUI should be highlighted in Bold.

So no way of styling IMPORTANT stuff, I'd have thought that would be as useful as highlighting properties values, etc. I'm not keen on using capitalisation to draw the readers attention to specific parts of the text, it sounds like the author is shouting at the reader! Can't we just assume that everything in this short extra note is important?

So that just leaves the other change requests I mentioned in my review.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jun 28, 2023

@mmusgrov We highlight exact names (properties, values, dependencies..) or parts of code with backticks. Parts of GUI should be highlighted in Bold.

So no way of styling IMPORTANT stuff, I'd have thought that would be as useful as highlighting properties values, etc. I'm not keen on using capitalisation to draw the readers attention to specific parts of the text, it sounds like the author is shouting at the reader! Can't we just assume that everything in this short extra note is important?

So that just leaves the other change requests I mentioned in my review.

I guess I am not following you in this.
We use NOTE, TIP, and WARNING markup to highlight specific bits. This capitalisation, as you wrote, adds a tool-tip-like image to the sentence and encapsulates it in a highlighted block. If you prefer to tune it down from WARNING to NOTE so that it will rendered as below, I can do it:
image

If you think we do not need a NOTE or any other highlighting about that issue, I can remove it completely so that it remains as just a finishing sentence.

I applied your suggestions to the text above. If you are not still OK with that, please add the suggestions directly to a particular line.

This whole text has a level 3 heading, I think it makes it all stand up in the guide.

@mmusgrov
Copy link
Contributor

@mmusgrov We highlight exact names (properties, values, dependencies..) or parts of code with backticks. Parts of GUI should be highlighted in Bold.
...

I only asked "Does the quarkus documentation style guide provide a recommendation for highlighting important text?" and the response was We highlight exact names (properties, values, dependencies..) or parts of code with backticks., ie no mention of capitalisation.

@mmusgrov
Copy link
Contributor

@zhfeng , adding one more revision when using @yrodiere exact wording without the bracket usage.

It is not the exact wording, you added commas around for example so now the grammar is incorrect, but Yoann's grammar was already correct. But let's just get this one merged.

I added the xref link you pointed out regarding the default (unnamed) datasource. These changes are already part of the main docs snapshot https://quarkus.io/version/main/guides/datasource#dev-services for few weeks now. Not sure why it is not part of the latest build yet.

[[jdbcstore]]
== Support for storing Quarkus transaction logs in a database

Transaction management can be configured to store transaction logs in a database by using a JDBC datasource.

Database storage of transaction logs generally performs worse than filesystem storage, but may be necessary in cloud environments lacking persistent storage, for example, if your application container cannot use persistent volumes.

To enable this capability, explicitly set `quarkus.transaction-manager.object-store.type` to `jdbc`.

You can also specify the datasource name for the transaction log storage by setting `quarkus.transaction-manager.object-store.datasource`.
If no configuration is provided, Quarkus uses the xref:datasource.adoc#dev-services[default datasource].

When the `quarkus.transaction-manager.object-store.create-table` configuration is enabled, the transaction log table gets automatically created if it does not already exist.

The code also includes

    @ConfigItem(defaultValue = "false")
    public boolean dropTable;

    @ConfigItem(defaultValue = "quarkus_")
    public String tablePrefix;

    @ConfigItem(defaultValue = "file-system")
    public ObjectStoreType type;

I think those need to be documented in addition to quarkus.transaction-manager.object-store.create-table, @zhfeng ?

Finally, I'm not asking for a change here but, for thoroughness I'd like to point out that the original issue description asked for:

Secondarily, schema creation control would need to be configurable. Currently Narayana will always check for the existence of the expected tables and attempt to create them when missing; it would be preferrable to expose some more flexibility, such as to log/generate the SQL scripts to create them instead, to allow controlling such aspects with schema evolution tools and/or potentially with different user credentials than the ones used at runtime.

I guess that could be implemented in a follow up issue in the future.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jun 28, 2023

@mmusgrov
One of many rules we should apply when writing technical documentation is not to use the example in brackets if the example can be explained without them. I checked it based on my own experience and an application for checking proper grammar, and it seems fine. @michelle-purcell Michelle, please, can I ask you for your review if you will have a minute? The text is below:

[[jdbcstore]]
== Support for storing Quarkus transaction logs in a database

Transaction management can be configured to store transaction logs in a database by using a JDBC datasource.

Database storage of transaction logs generally performs worse than filesystem storage.
Still, it may be necessary in cloud environments lacking persistent storage, for example, if your application container cannot use persistent volumes.

To enable this capability, explicitly set `quarkus.transaction-manager.object-store.type` to `jdbc`.

You can also specify the datasource name for the transaction log storage by setting `quarkus.transaction-manager.object-store.datasource`.
If no configuration is provided, Quarkus uses the xref:datasource.adoc#dev-services[default datasource].

When the `quarkus.transaction-manager.object-store.create-table` configuration is enabled, the transaction log table gets automatically created if it does not already exist.

To work around the known issue of Agroal having a different view on running transaction checks, set the datasource transaction type to `disabled`.

Regarding capitalization you asked earlier; now I understand.
I checked the https://quarkus.io/guides/doc-reference, and it is really not specified there.
But I see this as a good opportunity to add it there. Will put this on my TODO list.

But to answer your question, here is a little rule of thumb for each of these:
NOTE: For every mention that is above the current scope or frame but still adds value to the content.
An example can be a sentence one would start writing with "Note that.....". The note is also good for noting temporary issues or plans.
WARNING: For anything that could cause trouble.
TIP: An idea or a hint from a professional experience that could be handy, but it is not necessary to reach the goal of the procedure.

All these should be one-liners. In that case, the sentence starts with NOTE: text... asciidoc markup.
If we need to use one of these capitalizations with a snippet or a command, we put these into a block:

[NOTE]
====
* Text here

* Example code here
====

@zhfeng zhfeng force-pushed the issue_33739_doc branch from 6494ac0 to fc57ff6 Compare June 29, 2023 06:57
@mmusgrov
Copy link
Contributor

Thanks for the explanation; so the capitalised keywords MUST be enclosed in square brackets for them to affect the styling.

Regarding the grammar of Yoanns text; just replace the first bracket with a comma and remove the closing bracket.

@mmusgrov
Copy link
Contributor

Oops, I was using MUST in the RFC2119 sense, I wasn't making a point.

@mmusgrov
Copy link
Contributor

mmusgrov commented Jun 29, 2023

Does the quarkus documentation style guide provide a recommendation for highlighting important text? For example the EAP docs use special fonts and background for this purpose.

Ah, I notice that use of a capitalised keyword followed by a colon is asciidoc markup. I'm not a technical writer, and the Narayana project uses things like markdown and DocBook so I don't have the same level of knowledge of that markup that you guys do, there are way too many markup variants to keep track of which rules apply to which variant.

@mmusgrov
Copy link
Contributor

@mmusgrov One of many rules we should apply when writing technical documentation is not to use the example in brackets if the example can be explained without them. I checked it based on my own experience and an application for checking proper grammar, and it seems fine.

I'm always on the look-out to improve my written English, which isn't the best, so I'd like to take a look at that application, that said the way you have removed the brackets has definitely produced a grammatically incorrect sentence.

@mmusgrov
Copy link
Contributor

@mmusgrov One of many rules we should apply when writing technical documentation is not to use the example in brackets if the example can be explained without them. I checked it based on my own experience and an application for checking proper grammar, and it seems fine. @michelle-purcell Michelle, please, can I ask you for your review if you will have a minute? The text is below:

Michelle, bear in mind that I have asked for dropTable and tablePrefix to also be documented so the text will change.

@michelle-purcell
Copy link
Contributor

@MichalMaler - Sure. I'll ping you in a moment direct for some context catchup while reviewing.... 👍

@mmusgrov - Regarding the question in : #34297 (comment)

Does the quarkus documentation style guide provide a recommendation for highlighting important text? For example, the EAP docs use special fonts and background for this purpose.

The Quarkus contributor style guide does not clarify how to highlight specific text as important that must stand out from the rest.

But this is covered in the linked asciidoc documentation. Quarkus docs supports all of the asciidoc admonition elements, including IMPORTANT.

[IMPORTANT]
====
<The important text goes here>
====

or (when you only have 1 paragraph of text) the simple form:

IMPORTANT: <The important text goes here>

The Quarkus stylesheet renders the ^^ markup elements like so:

image

If you'd like to see an update to the stylesheet, asciidoc transform job, or the contributor guide around this topic, the best approach is to open a separate GitHub issue for each item. Thanks

@michelle-purcell
Copy link
Contributor

#34297 (comment)

@mmusgrov - I agree with @MichalMaler regarding the suggestion to drop the parenthesis ( ) from the sentence in #34297 (comment).

While the sentence is grammatically correct and understandable in English grammar, in technical writing for global audiences, we typically try to avoid using parenthetical statements on the premise that if something is important enough to be in the sentence, it should be fully part of that sentence. The meaning might also be mistranslated... but that's another story.

Examples tend to be really important parts of technical documentation that the reader uses, compared with supplemental information that you might also commonly find in parentheses.

An example of a parenthesis use case could be when we want to say something like "Jakarta Persistence (formerly known as JPA) is bla bla..." The formerly bit isn't as essential as a technical example.

Hope that helps 🌞

@mmusgrov
Copy link
Contributor

mmusgrov commented Jun 29, 2023

Still, it may be necessary in cloud environments lacking persistent storage, for example, if your application container cannot use persistent volumes.

My comment was that the new sentence that replaces the brackets is grammatically incorrect.

@mmusgrov
Copy link
Contributor

@MichalMaler - Sure. I'll ping you in a moment direct for some context catchup while reviewing.... 👍
...

If you'd like to see an update to the stylesheet, asciidoc transform job, or the contributor guide around this topic, the best approach is to open a separate GitHub issue for each item. Thanks

That's ok, I followed up on this part of thread earlier with: #34297 (comment)

@MichalMaler
Copy link
Contributor

All righty then. Here I have one more version I like the most. Thank you so much, @michelle-purcell for your input and suggestions regarding this draft.

This could be used as a backbone for the further content adding:

In cloud environments where persistent storage is not available, such as when application containers are unable to use persistent volumes, you can configure the transaction management to store transaction logs in a database by using a JDBC datasource.

IMPORTANT: While there are several benefits to using a database to store translation logs you might notice a reduction in performance compared with using the file system to store the logs.

To configure a Quarkus JDBC datasource for transaction logging, set the `quarkus.transaction-manager.object-store.type` configuration property to `jdbc`.

You can also specify a name for the datasource for the transaction log storage by setting `quarkus.transaction-manager.object-store.datasource`.
If no configuration is provided, Quarkus uses the xref:datasource.adoc#dev-services[default datasource].

When you enable the `quarkus.transaction-manager.object-store.create-table` configuration property, the transaction log table gets automatically created if it does not already exist.

To work around the 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 to `disabled`.

@zhfeng
Copy link
Contributor Author

zhfeng commented Jun 30, 2023

@MichalMaler Is this a final version?

@mmusgrov
Copy link
Contributor

IMPORTANT: While there are several benefits to using a database to store translation logs you might notice a reduction in performance compared with using the file system to store the logs.

typo: translation logs should be transaction logs.

I also asked for some changes in comment: #34297 (comment)

Otherwise the new text is good.

@MichalMaler
Copy link
Contributor

@zhfeng I will try to incorporate @mmusgrov suggestions that are still pending and then I think this is good to go :)
Let me have a look at it. Will ping you when ready.
Thank you.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jul 4, 2023

@zhfeng Here it is.

[[jdbcstore]]
== Configure storing of Quarkus transaction logs in a database

In cloud environments where persistent storage is not available, such as when application containers are unable to use persistent volumes, you can configure the transaction management to store transaction logs in a database by using a JDBC datasource.

IMPORTANT: While there are several benefits to using a database to store transaction logs, you might notice a reduction in performance compared with using the file system to store the logs.

Quarkus allows the following JDBC-specific configuration of the object store included in `quarkus.transacion-manager.object-store.<property>` properties, where <property> can be:

* `type` (_string_): Configure this property to `jdbc` to enable usage of a Quarkus JDBC datasource for transaction logging.
* `datasource` (_string_): Specifies the name of the datasource for the transaction log storage.
If no value is provided for the `datasource` property, Quarkus uses the xref:datasource.adoc#dev-services[default datasource].
* `create-table` (_boolean_): When set to `true`, the transaction log table gets automatically created if it does not already exist. 
* `drop-table` (_boolean_): To drop the table on startup if it exists.
* `table-prefix` (string): The prefix to apply to a related table name.
The default is `quarkus_`.

[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 that is used to write transaction logs to `disabled`:

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

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

@MichalMaler
Copy link
Contributor

@mmusgrov Hello Michael. Can I ask you for the final check, please?

@mmusgrov
Copy link
Contributor

mmusgrov commented Jul 5, 2023

Quarkus allows the following JDBC-specific configuration of the object store included in quarkus.transacion-manager.object-store.<property> properties, where can be:

Angle brackets are used in markdown to turn a URL or email address into a link so <property> will not render correctly.

On the first version of this PR I noticed that the build provided a link to the final rendered output but that link didn't get updated so I cannot validate that the markdown produces reasonable output.

  • type (string): Configure this property to jdbc to enable usage of a Quarkus JDBC datasource for transaction logging.

Will you specify the defaults on all the properties, similar to how you do with the datasource and table-prefix properties. Not having or not knowing the defaults typically results in developers specifying values for every property so the "Convention over Configuration" principle cannot be applied and it impacts performance.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jul 11, 2023

Quarkus allows the following JDBC-specific configuration of the object store included in quarkus.transacion-manager.object-store.<property> properties, where can be:

Angle brackets are used in markdown to turn a URL or email address into a link so <property> will not render correctly.

On the first version of this PR I noticed that the build provided a link to the final rendered output but that link didn't get updated so I cannot validate that the markdown produces reasonable output.

  • type (string): Configure this property to jdbc to enable usage of a Quarkus JDBC datasource for transaction logging.

Will you specify the defaults on all the properties, similar to how you do with the datasource and table-prefix properties. Not having or not knowing the defaults typically results in developers specifying values for every property so the "Convention over Configuration" principle cannot be applied and it impacts performance.

Hello, @mmusgrov. I hope you are doing well.

Sure, I will add the defaults for all non-boolean properties. I will prepare the final version, and then I will be ready to merge this and continue in the standard way. If needed, I will follow up, and the reviewers can provide direct suggestions for specific lines.

In AsciiDoc and Red Hat docs, we have a practice of using angled brackets, like "<PutYourValueHere>", to indicate a space that requires user input. It will render as you see it.

@mmusgrov
Copy link
Contributor

Sure, I will add the defaults for all non-boolean properties. I will prepare the final version, and then I will be ready to merge this and continue in the standard way. If needed, I will follow up, and the reviewers can provide direct suggestions for specific lines.

Sounds good.

In AsciiDoc and Red Hat docs, we have a practice of using angled brackets, like "<PutYourValueHere>", to indicate a space that requires user input. It will render as you see it.

I noticed it because it didn't render when I ran it through pandoc, that's why I mentioned that the first version of the PR produced a link to the final rendered output thus giving me the opportunity to check as opposed to my having to review a comment. Now if you are saying it will render correctly then I'll trust you, I guess I'll need to hold back on my approval until you push an update, unless one of the other reviewers approves it.

@MichalMaler
Copy link
Contributor

@mmusgrov Hello. Just in case @zhfeng is on PTO, I created a PR with these changes and with a cross-file link to the related Datasource Narayana section, as suggested by @yrodiere .
#34699
If you want to merge this PR, I am for closing this one. However, I will also add the updated version here in the comment.
Cheers.

@MichalMaler
Copy link
Contributor

[[jdbcstore]]
== Configure storing of Quarkus transaction logs in a database

In cloud environments where persistent storage is not available, such as when application containers are unable to use persistent volumes, you can configure the transaction management to store transaction logs in a database by using a JDBC datasource.

IMPORTANT: While there are several benefits to using a database to store transaction logs, you might notice a reduction in performance compared with using the file system to store the logs.

Quarkus allows the following JDBC-specific configuration of the object store included in `quarkus.transacion-manager.object-store.<property>` properties, where <property> can be:

* `type` (_string_): Configure this property to `jdbc` to enable usage of a Quarkus JDBC datasource for transaction logging.
The default value is `file-system`.
* `datasource` (_string_): Specify the name of the datasource for the transaction log storage.
If no value is provided for the `datasource` property, Quarkus uses the xref:datasource.adoc#dev-services[default datasource].
* `create-table` (_boolean_): When set to `true`, the transaction log table gets automatically created if it does not already exist.
The default value is `false`.
* `drop-table` (_boolean_): When set to `true`, the table creation is canceled on startup if it exists.
The default value is `false`.
* `table-prefix` (string): Specify the prefix for a related table name.
The default value is `quarkus_`.

[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.
====

@mmusgrov
Copy link
Contributor

mmusgrov commented Jul 12, 2023

  • drop-table (boolean): When set to true, the table creation is canceled on startup if it exists.

This seems to imply that there will be no attempt to create the tables whereas the actual meaning is that the tables will be dropped on startup if they exist.

Can someone else please provide the approval for this PR, @jmartisk or @yrodiere if either of you are happy with the latest text then can you approve so that we can finally get this one merged.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jul 13, 2023

the tables will be dropped on startup if they exist

I updated my PR with your last request. Asking Yoann and Jan to merge it.
This one can be closed.

@yrodiere
Copy link
Member

Superseded by #34699, closing.

Thanks all for your insight!

@yrodiere yrodiere closed this Jul 13, 2023
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants