-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
🙈 The PR is closed and the preview is expired. |
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.
|
There was a problem hiding this 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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`. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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:
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
@mmusgrov @yrodiere
|
I think Martin's expertise is with LRA rather than the transaction log store types.
See my earlier comment about using the term
Yoann had a comment and suggestion about using the term JDBCStore.
|
@mmusgrov is correct, but I agree with the suggestions reviewers made. Otherwise, it makes sense to me. |
@zhfeng Here is the reworked version that containts @mmusgrov and @yrodiere suggestions.
|
Is this meant to close #33739? If so, please update the description so that issue gets closed when this gets merged. Thanks |
I don't think the
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).
@zhfeng is the default still
Is it documented elsewhere what the default is?
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. |
There was a problem hiding this 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
Yes, the journal based store is not supported in quarkus right now. Only
Yeah, it is.
I think it could be https://quarkus.io/guides/datasource#default-datasource
|
@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
|
@mmusgrov We highlight exact names (properties, values, dependencies..) or parts of code with backticks. |
So no way of styling So that just leaves the other change requests I mentioned in my review. |
I guess I am not following you in this. 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. |
I only asked "Does the quarkus documentation style guide provide a recommendation for highlighting important text?" and the response was |
It is not the exact wording, you added commas around
The code also includes
I think those need to be documented in addition to 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:
I guess that could be implemented in a follow up issue in the future. |
@mmusgrov
Regarding capitalization you asked earlier; now I understand. But to answer your question, here is a little rule of thumb for each of these: All these should be one-liners. In that case, the sentence starts with
|
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. |
Oops, I was using MUST in the RFC2119 sense, I wasn't making a point. |
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 |
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. |
Michelle, bear in mind that I have asked for |
@MichalMaler - Sure. I'll ping you in a moment direct for some context catchup while reviewing.... 👍 @mmusgrov - Regarding the question in : #34297 (comment)
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.
or (when you only have 1 paragraph of text) the simple form:
The Quarkus stylesheet renders the ^^ markup elements like so: 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 |
@mmusgrov - I agree with @MichalMaler regarding the suggestion to drop the parenthesis 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 🌞 |
My comment was that the new sentence that replaces the brackets is grammatically incorrect. |
That's ok, I followed up on this part of thread earlier with: #34297 (comment) |
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:
|
@MichalMaler Is this a final version? |
typo: I also asked for some changes in comment: #34297 (comment) Otherwise the new text is good. |
@zhfeng Here it is.
|
@mmusgrov Hello Michael. Can I ask you for the final check, please? |
Angle brackets are used in markdown to turn a URL or email address into a link so 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.
Will you specify the defaults on all the properties, similar to how you do with the |
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 " |
Sounds good.
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. |
@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 . |
|
This seems to imply that there will be no attempt to create the tables whereas the actual meaning is that 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. |
I updated my PR with your last request. Asking Yoann and Jan to merge it. |
Superseded by #34699, closing. Thanks all for your insight! |
It's related to #33739