[WFLY-19790] Remove batch-processing DS file #960

liweinan · 2024-09-30T14:52:16Z

Issue: https://issues.redhat.com/browse/WFLY-19790

liweinan · 2024-09-30T15:57:09Z

@jfdenise I don't know how to fix this error:

https://github.com/wildfly/quickstart/actions/runs/11109598030/job/30865320646?pr=960#step:6:6605

As the h2-database has already been added. Could you please help check this? Thanks!

batch-processing/configure-server.cli

emmartins · 2024-10-03T11:48:30Z

Besides the script change, guess we need instructions wrt the script execution when used with bare-metal standard distribution, and a config restore script too? You can reuse content from

quickstart/ee-security/README-source.adoc

Line 40 in 484ba35

=== Configure the Server

emmartins · 2024-10-03T11:49:42Z

Btw I really thought you would just need to update the persistence.xml, ie no need for config change script...
@scottmarlow I wonder if this (replace usage of the DS file with just persistence.xml) would be possible?

liweinan · 2024-10-07T11:27:58Z

@emmartins Thanks for checking this PR!

I discussed this issue with @jamezp, and the DS file may still be used in the future, so I am holding off on this work.

emmartins · 2024-10-28T13:03:44Z

@jamezp can you please clarify what @liweinan said above about new use for DS files? We have those deprecated for long, and we are logging a log warning (the reason for this issue) when deploying such files...

jamezp · 2024-10-28T16:01:56Z

I do understand the log message is a bit annoying and really we should have a look at that. There are quite a few comments on https://issues.redhat.com/browse/WFLY-4296 as to why this support should not be removed.

The problem is there is no good way to create a test data source without the *-ds.xml files.

One option is to simply use the default data source, however I'm not sure if we purposely do not do that in Quickstarts for a reason.

liweinan · 2024-10-29T10:10:37Z

One option is to simply use the default data source

btw here is one blog post that I'm writing on this topic: wildfly/wildfly.org#677

If this is a better solution for this example, I can work on it.

emmartins · 2024-10-29T11:43:40Z

really up to you guys what you want to use as datasource, be it standard or not I'm fine with it, just agree with QE it's no longer ok to use a -ds file to config a database, you now have the standard persistence.xml and DatasourceDefinition annotation that should be the alternative, or at least the first alternative, if the vendor requires something more (like a cli script) then be it

liweinan · 2024-10-30T14:35:52Z

@emmartins Thanks for the comment!

According to all your opinions, I'd prefer to use the CLI script to do this.

James has provided an example showing the CLI usage here: jberet/jberet-examples#13

I'll use the above example as a reference and update the PR.

liweinan · 2024-10-30T17:28:13Z

@emmartins I have replaced Glow with the data source feature pack in combination with the CLI script to create the database and configure the datasource. Could you please check if this solution is acceptable? Thanks!

liweinan · 2024-10-30T17:29:08Z

@jamezp I'm not sure how to fix the Kubernetes CI test failure. Could you please help to check it when you have time? Thanks!

jamezp · 2024-10-30T17:39:58Z

@liweinan I have no idea why the Kubernetes tests are failing. There seems to be no indication as to why.

emmartins · 2024-10-30T18:13:51Z

batch-processing/pom.xml

@@ -200,12 +203,32 @@
                        <groupId>org.wildfly.plugins</groupId>
                        <artifactId>wildfly-maven-plugin</artifactId>
                        <configuration>
-                            <discover-provisioning-info>


why are we going back to directly using galleon layers, instead of glow's discovery? This seems a mistake

Maybe you are just doing copy paste of outdated (in the sense of not using glow) configuration? You can use glow and pack a script. In short the only change you do here is to add

<packaging-scripts> <packaging-script> <scripts> <script>${configure.ds.script}</script> </scripts>  <resolve-expressions>false</resolve-expressions> </packaging-script> </packaging-scripts>

emmartins · 2024-10-30T19:00:12Z

batch-processing/add-h2-ds.cli

@@ -0,0 +1 @@
+xa-data-source add --name=batch_db --enabled=true --use-java-context=true --use-ccm=true --jndi-name=java:jboss/datasources/batch-processingDS --xa-datasource-properties={"URL"=>"jdbc:h2:mem:batch-processing;DB_CLOSE_ON_EXIT=FALSE;DB_CLOSE_DELAY=-1"} --driver-name=h2 --password=sa --user-name=sa --same-rm-override=false --no-recovery=true


I still think you could do this op using only Jakarta specs, which would make this app portable among vendors. But I tried to use a DataSourceDefinition like the one below (you can add this for instance to Contact class) but first Glow complained and required me to prevent fail on error, and then boot failed with a CNFE for org.h2.jdbcx.JdbcDataSource. I guess we can try to explore the specs way and clarify how should users use the specs DataSourceDefinition later...

@DataSourceDefinition(name="java:jboss/datasources/batch-processingDS",
className="org.h2.jdbcx.JdbcDataSource",
url="jdbc:h2:mem:batch-processing;DB_CLOSE_ON_EXIT=FALSE;DB_CLOSE_DELAY=-1",
user="sa",
password="sa"
)

nevermind, I was able to make it work, I will send an alternative PR for you to have a look

emmartins · 2024-10-30T19:02:39Z

batch-processing/pom.xml

-                                    <addOn>h2-database:default</addOn>
-                                </addOns>
-                            </discover-provisioning-info>
+                            <feature-packs>


we should revert this change too, other than adding the script of course

emmartins

Please see my comments

emmartins · 2024-10-30T20:17:33Z

@liweinan @jamezp alternative PR using the specs DataSourceDefinition at #973

liweinan · 2024-10-31T01:13:20Z

@emmartins Thanks for the review! I'll check it.

alternative PR using the specs DataSourceDefinition at #973

Cool! I'll see if this can work with this example.

liweinan · 2024-10-31T17:45:19Z

I closed this PR because it's been replaced by the PR mentioned above.

liweinan requested a review from emmartins as a code owner September 30, 2024 14:52

liweinan changed the title ~~WFLY-19790 Remove batch-processing DS file~~ [WFLY-19790] Remove batch-processing DS file Sep 30, 2024

liweinan force-pushed the WFLY-19790 branch from 29e31c5 to 626cce3 Compare September 30, 2024 15:46

liweinan marked this pull request as draft October 1, 2024 15:15

emmartins requested changes Oct 3, 2024

View reviewed changes

batch-processing/configure-server.cli Outdated Show resolved Hide resolved

liweinan force-pushed the WFLY-19790 branch from 626cce3 to 96ce3e9 Compare October 30, 2024 15:55

liweinan marked this pull request as ready for review October 30, 2024 17:08

[WFLY-19790] Remove batch-processing DS file

dd92f9c

liweinan force-pushed the WFLY-19790 branch from ff4ea78 to dd92f9c Compare October 30, 2024 17:10

emmartins reviewed Oct 30, 2024

View reviewed changes

emmartins requested changes Oct 30, 2024

View reviewed changes

liweinan closed this Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFLY-19790] Remove batch-processing DS file #960

[WFLY-19790] Remove batch-processing DS file #960

liweinan commented Sep 30, 2024 •

edited by emmartins

Loading

liweinan commented Sep 30, 2024

emmartins commented Oct 3, 2024

emmartins commented Oct 3, 2024

liweinan commented Oct 7, 2024 •

edited

Loading

emmartins commented Oct 28, 2024

jamezp commented Oct 28, 2024

liweinan commented Oct 29, 2024

emmartins commented Oct 29, 2024 •

edited

Loading

liweinan commented Oct 30, 2024 •

edited

Loading

liweinan commented Oct 30, 2024

liweinan commented Oct 30, 2024

jamezp commented Oct 30, 2024

emmartins Oct 30, 2024

emmartins Oct 30, 2024 •

edited

Loading

emmartins Oct 30, 2024 •

edited

Loading

emmartins Oct 30, 2024

emmartins Oct 30, 2024

emmartins left a comment

emmartins commented Oct 30, 2024

liweinan commented Oct 31, 2024

liweinan commented Oct 31, 2024

		@@ -0,0 +1 @@
		xa-data-source add --name=batch_db --enabled=true --use-java-context=true --use-ccm=true --jndi-name=java:jboss/datasources/batch-processingDS --xa-datasource-properties={"URL"=>"jdbc:h2:mem:batch-processing;DB_CLOSE_ON_EXIT=FALSE;DB_CLOSE_DELAY=-1"} --driver-name=h2 --password=sa --user-name=sa --same-rm-override=false --no-recovery=true

[WFLY-19790] Remove batch-processing DS file #960

[WFLY-19790] Remove batch-processing DS file #960

Conversation

liweinan commented Sep 30, 2024 • edited by emmartins Loading

liweinan commented Sep 30, 2024

emmartins commented Oct 3, 2024

emmartins commented Oct 3, 2024

liweinan commented Oct 7, 2024 • edited Loading

emmartins commented Oct 28, 2024

jamezp commented Oct 28, 2024

liweinan commented Oct 29, 2024

emmartins commented Oct 29, 2024 • edited Loading

liweinan commented Oct 30, 2024 • edited Loading

liweinan commented Oct 30, 2024

liweinan commented Oct 30, 2024

jamezp commented Oct 30, 2024

emmartins Oct 30, 2024

Choose a reason for hiding this comment

emmartins Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

emmartins Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

emmartins Oct 30, 2024

Choose a reason for hiding this comment

emmartins Oct 30, 2024

Choose a reason for hiding this comment

emmartins left a comment

Choose a reason for hiding this comment

emmartins commented Oct 30, 2024

liweinan commented Oct 31, 2024

liweinan commented Oct 31, 2024

liweinan commented Sep 30, 2024 •

edited by emmartins

Loading

liweinan commented Oct 7, 2024 •

edited

Loading

emmartins commented Oct 29, 2024 •

edited

Loading

liweinan commented Oct 30, 2024 •

edited

Loading

emmartins Oct 30, 2024 •

edited

Loading

emmartins Oct 30, 2024 •

edited

Loading