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

[WFLY-7418] jberet-jpa-repository integration in wildfly batch #16085

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

amoscatelli
Copy link

@amoscatelli amoscatelli commented Sep 20, 2022

Hi everybody,
this pull request brings integration with the new jberet-jpa-repository

As the name says, that's a jberet repository based on jpa.
Being based on jakarta.persistence by default, it offers a broader support for databases and can rely on first or second level caches for performance.

Also, it is a consequential fix/workaround for :

jberet/jsr352#142

Related to :
https://issues.redhat.com/browse/WFLY-7418
https://developer.jboss.org/thread/272830

I am been using a similar integration (for Wildfly 26.X) in my production environment for the last 9 monthes.
I waited for Wildfly Batch subsystem to be migrated to jakarta.persistance before preparing this pull request.

It seems I can't sign in with sso.redhat at the moment, so I can't update or create any new issue.

Does this seem good to you ?
Is there any cleaner way to get a boostraped instance of hibernate in a wildfly subsystem ?

Also, I enabed the jpa repository only for the 3.0 xml definition. Is this ok (or you want it for 2.0 or 1.0 too) ?

Thank you in advance

Conflicts:
	batch-jberet/src/main/java/org/wildfly/extension/batch/jberet/deployment/BatchDeploymentDescriptorParser_1_0.java
	pom.xml
Conflicts:
	batch-jberet/pom.xml
	ee-feature-pack/common/pom.xml
	ee-feature-pack/common/src/main/resources/modules/system/layers/base/org/wildfly/extension/batch/jberet/main/module.xml
	ee-feature-pack/pom.xml
@wildfly-ci
Copy link

Hello, amoscatelli. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

@chengfang chengfang added Feature PR provides a new feature 28.x labels Sep 20, 2022
@amoscatelli
Copy link
Author

@chengfang I think that's it !
What do you think ?

I migrated to Wildfly 27.0.0.Final and this integration is working in our test and production environments.

@amoscatelli
Copy link
Author

I also created an issue to track this :
https://issues.redhat.com/browse/WFLY-17340

@alessandro-tucci-visiontech

@chengfang , as a subscriber to this pull request, I can confirm that I would be glad to see it merged into the official wildfly main branch. We come from the corresponding non-jakarta version. Since we are migrating to jakarta, we tried to adopt this integration as a "custom module". By this, I can confirm that it is working in our test environment.

TBH, the test results would be compatible with the decision of moving it to our production systems, too.
However, we'd rather wait for its official release by the Wildfly team before taking this step

@bstansberry bstansberry removed the 28.x label Apr 18, 2023
@francescopotenziani
Copy link

Hi everybody,
any updates about this integration?

/**
* Represents a JPA job repository.
*
* @author <a href="mailto:[email protected]">James R. Perkins</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this need to be changed to your email address.

<dependency>
<groupId>org.jberet</groupId>
<artifactId>jberet-jpa-repository</artifactId>
<version>2.1.3.Final</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

The version should be defined into pom.xml and use property here.

@liweinan
Copy link
Contributor

liweinan commented Jan 2, 2024

I'll take this. @amoscatelli Thanks for the PR! I have put some review comments in above. Besides it needs rebase.

In addition, do you have a demo project showing its usage of the new feature? Or you may need to integration test case like this:

@jamezp @bstansberry As this is a new feature, do we need to go through the whole new feature process? (I'm starting from writing the proposal in: https://github.com/wildfly/wildfly-proposals/tree/main/batch)

@jamezp jamezp added rebase-this PR has a merge conflict. missing-reqs Features missing one or more of the pre-merge requirements labels Jan 3, 2024
Copy link
Member

@jamezp jamezp left a comment

Choose a reason for hiding this comment

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

The commits should be squashed when you're comfortable doing so.

There also needs to be a model version bump and a schema bump. This bump should be done in it's own commit and be the first commit. An example is 145400f.

Comment on lines 1 to 21
/*
* JBoss, Home of Professional Open Source.
* Copyright 2015, Red Hat, Inc., and individual contributors
* as indicated by the @author tags. See the copyright.txt file in the
* distribution for a full listing of individual contributors.
*
* This is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as
* published by the Free Software Foundation; either version 2.1 of
* the License, or (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/
Copy link
Member

Choose a reason for hiding this comment

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

This is an old copyright and needs to be:

/*
 * Copyright The WildFly Authors
 * SPDX-License-Identifier: Apache-2.0
 */

Copy link
Author

Choose a reason for hiding this comment

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

@francescopotenziani can you update this ?

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

This file should not be edited. A new file named wildfly-batch-jberet_4.0.xsd should be created.

Copy link
Author

Choose a reason for hiding this comment

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

Understood. Seems legit, this was one of the questions I asked previously but I got no answer

@jamezp
Copy link
Member

jamezp commented Jan 3, 2024

I'm also not quite sure what the JIRA has to do with this feature. The JIRA title is "Batch deployments with a large number of executed jobs can lock up or slow down the web console" and this seems to be about adding a JPA repository.

@amoscatelli
Copy link
Author

amoscatelli commented Jan 3, 2024

I do not work for the company I made this PR for any longer.
This was one and half year ago.
@francescopotenziani will work on this on my behalf and I gave him permissions on the repo.

I'll try to help and answer on the review anyway.

@liweinan
Copy link
Contributor

liweinan commented Jan 3, 2024

@amoscatelli Thanks for the update!

@jamezp
Copy link
Member

jamezp commented Jan 3, 2024

@liweinan This could probably use the WildFly process as well https://www.wildfly.org/news/2023/11/22/WildFly_Feature_Development_Process/.

@liweinan
Copy link
Contributor

liweinan commented Jan 4, 2024

@jamezp Okay!

@alessandro-tucci-visiontech

@liweinan @jamezp @bstansberry Please let me introduce myself: I'm Alessandro Tucci, I'm assisting @francescopotenziani with the pull request. Sometimes, I'll be providing some answers on his behalf, especially when some tighter feedback is needed and he is overwhelmed with tech efforts. Instead, for what concerns code edits, he will be the only one making them

@alessandro-tucci-visiontech

I'll take this. @amoscatelli Thanks for the PR! I have put some review comments in above. Besides it needs rebase.

For internal usage, we've already merged the git node right after the one tagged with 30.0.1.Final (SHA-1: d130fbc) with the latest commit of this PR, in order to:

  • check whether this integration worked with the official 30.0.1.Final Wildfly version
  • be able to use it at least internally.

Additionally, for the sake of the PR process, we're currently merging the latest commit node of the Wildfly code base (SHA-1: aaf99a4) with the latest commit of this PR. Please let us know:

  • whether this approach is correct
  • whether a specific merge strategy is required on your side (to be precise, in this specific phase, we are merging wildfly/wildlfy into amoscatelly/wildfly)

@liweinan
Copy link
Contributor

liweinan commented Jan 4, 2024

@alessandro-tucci-visiontech Could you please provide test cases or demo project that based on this branch that can verifying the usability of this new feature?

Francesco added 4 commits January 4, 2024 15:59
…/pom.xml and ee-feature-pack/common/src/main/resources/modules/system/layers/base/org/wildfly/extension/batch/jberet/main/module.xml. Created new file wildfly-batch-jberet_4.0.xsd. Changed copyright with the new one. Defined the jberet-jpa-repository version in pom.xml.
Copy link

wildfly-bot bot commented Jan 9, 2024

Failed format check on this pull request:

  • None of the commit messages satisfy the following regex pattern: [WFLY-\d+]

Please fix the format according to these guidelines.

@wildfly-bot wildfly-bot bot requested a review from chengfang January 9, 2024 09:57
@liweinan
Copy link
Contributor

I'll verify this PR and make necessary modifications.

@liweinan
Copy link
Contributor

@liweinan
Copy link
Contributor

I start to verify this PR by building the branch, and edit the standalone.xml like this:

<subsystem xmlns="urn:jboss:domain:batch-jberet:4.0">
            <default-job-repository name="jpa"/>
            <!-- <default-job-repository name="in-memory"/> -->
            <default-thread-pool name="batch"/>
            <security-domain name="ApplicationDomain"/>
            <job-repository name="jpa">
                <jpa data-source="ExampleDS"/>
            </job-repository>
            <!-- <job-repository name="in-memory">
                <in-memory/>
            </job-repository> -->
            <thread-pool name="batch">
                <max-threads count="10"/>
                <keepalive-time time="30" unit="seconds"/>
            </thread-pool>
        </subsystem>
        <subsystem xmlns="urn:jboss:domain:bean-validation:1.0"/>
        <subsystem xmlns="urn:jboss:domain:core-management:1.0"/>
        <subsystem xmlns="urn:jboss:domain:datasources:7.1">
            <datasources>
                <datasource jndi-name="java:jboss/datasources/ExampleDS" pool-name="ExampleDS" enabled="true" use-java-context="true" statistics-enabled="${wildfly.datasources.statistics-enabled:${wildfly.statistics-enabled:false}}">
                    <connection-url>jdbc:h2:mem:test;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=FALSE;MODE=${wildfly.h2.compatibility.mode:REGULAR}</connection-url>
                    <driver>h2</driver>
                    <security user-name="sa" password="sa"/>
                </datasource>
                <drivers>
                    <driver name="h2" module="com.h2database.h2">
                        <xa-datasource-class>org.h2.jdbcx.JdbcDataSource</xa-datasource-class>
                    </driver>
                </drivers>
            </datasources>
        </subsystem>

With the above configuration I start the server, and here is the output:

02:02:17,681 INFO  [org.jboss.as.clustering.infinispan] (ServerService Thread Pool -- 78) WFLYCLINF0002: Started org.jberet.jpa.repository.entity.JobExecutionJpa cache from hibernate container
02:02:17,691 INFO  [org.jboss.as.clustering.infinispan] (ServerService Thread Pool -- 78) WFLYCLINF0002: Started org.jberet.jpa.repository.entity.JobExecutionJpa-pending-puts cache from hibernate container
02:02:17,697 INFO  [org.jboss.as.clustering.infinispan] (ServerService Thread Pool -- 78) WFLYCLINF0002: Started org.jberet.jpa.repository.entity.StepExecutionJpa cache from hibernate container
02:02:17,699 INFO  [org.jboss.as.clustering.infinispan] (ServerService Thread Pool -- 78) WFLYCLINF0002: Started org.jberet.jpa.repository.entity.StepExecutionJpa-pending-puts cache from hibernate container
02:02:17,702 INFO  [org.jboss.as.clustering.infinispan] (ServerService Thread Pool -- 78) WFLYCLINF0002: Started org.jberet.jpa.repository.entity.JobInstanceJpa cache from hibernate container
02:02:17,706 INFO  [org.jboss.as.clustering.infinispan] (ServerService Thread Pool -- 78) WFLYCLINF0002: Started org.jberet.jpa.repository.entity.JobInstanceJpa-pending-puts cache from hibernate container
02:02:17,710 INFO  [org.jboss.as.clustering.infinispan] (ServerService Thread Pool -- 78) WFLYCLINF0002: Started org.jberet.jpa.repository.entity.PartitionExecutionJpa cache from hibernate container
02:02:17,714 INFO  [org.jboss.as.clustering.infinispan] (ServerService Thread Pool -- 78) WFLYCLINF0002: Started org.jberet.jpa.repository.entity.PartitionExecutionJpa-pending-puts cache from hibernate container

I deployed the example here:

Then I access the service:

➤ curl http://localhost:8080/batch-deployment/batch/start

However the job get stuck in STARTING status:

image

If I switch to in-memory job repository then the job get completed:

image

I tried to use jdbc job repository and it seems to have the same problem with the jpa one. I guess this is not relevant to the PR and it's related to the WildFly version used in the PR. I'll try to rebase this PR to other working branch to verify it.

@liweinan
Copy link
Contributor

I have rebased the PR to main branch here: #17526

@liweinan
Copy link
Contributor

@alessandro-tucci-visiontech It seems I met problem when using the JPA repo. Besides I have rebased this PR to the main branch of WildFly

Could you please have a look at it? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature PR provides a new feature missing-reqs Features missing one or more of the pre-merge requirements rebase-this PR has a merge conflict.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants