Added support for dynamic spring batch job invoication based on message header #968

jlpedrosa · 2016-04-28T14:20:26Z

Pull request for issue: https://issues.apache.org/jira/browse/CAMEL-9733

…prinbatch component https://issues.apache.org/jira/browse/CAMEL-9733

davsclaus · 2016-04-28T14:28:39Z

Can you run checkstyle and fix those issues

mvn compile -Psourcecheck

in the directory with the component.

Also the header name should use naming: CamelComponentNameHeaderName, eg CamelSpringBatchXXXXX

trohovsky · 2016-04-28T14:41:49Z

...-spring-batch/src/main/java/org/apache/camel/component/spring/batch/SpringBatchEndpoint.java

@@ -83,7 +85,7 @@ protected void doStart() throws Exception {
        if (jobLauncher == null) {
            jobLauncher = resolveJobLauncher();
        }
-        if (job == null && jobName != null) {
+        if (job == null && jobName != null && jobName.compareTo("dynamic")!=0) {


"dynamic".equals(jobName)? But, what is wrong on setting initial jobName in URI path? AFAIK this concept is not used anywhare in Camel, so please remove the check for "dynamic".

Hi as jobName is the path and can't be made null beacause it's mandatory, "dynamic" keyword is used to omit the lookup of the Job and allow dynamic lookup based on message. Due to camel URI schema it's IMPOSSIBLe as you said in the ticket to make it null, so I need a keyword to know when it should be ignored. please check the ticket and your answer.

davsclaus · 2016-04-28T14:52:53Z

Can't you just use a dummy job name, and if there is a header that header take precedence. Or is there some pre-validation in the producer that a job with that name must exists?

trohovsky · 2016-04-28T15:01:34Z

...-spring-batch/src/main/java/org/apache/camel/component/spring/batch/SpringBatchProducer.java

+            }
+        }
+
+        if (job2run == null) {


This check is rednudant, this.job is never null.

No it's not redundant. if the jobName is dynamic, then the job will be set to null by the Endpoint.

jlpedrosa · 2016-04-28T15:07:39Z

Hi, I was not aware of -Psourcecheck flag, I've added a new commit with the changes to correct the style warnings, sorry for the inconvenience.

Can't you just use a dummy job name,
that is exactly what it is a word to avoid in the enpoint looking for the job...
look this is current URI for springbatch
from("direct:start").to("spring-batch:mockJob").to("mock:test");

where jobName is mockJob so I can't add this feature without breaking the backwards compatibility.. as jobName is the UriPath and it's mandatory, I can't just remove it, so I use "dynamic" As a dummy job from("direct:dynamic").to("spring-batch:dynamic").

trohovsky · 2016-04-28T15:08:06Z

...spring-batch/src/main/java/org/apache/camel/component/spring/batch/SpringBatchComponent.java

@@ -25,6 +25,7 @@
 public class SpringBatchComponent extends UriEndpointComponent {

    private static final String DEFAULT_JOB_LAUNCHER_REF_NAME = "jobLauncher";
+    public static final String DYNAMIC_JOBNAME = "DYNAMIC_JOBNAME_HEADER";


public static final String JOB_NAME = "CamelSpringBoot.jobName";

jlpedrosa · 2016-04-28T15:19:43Z

can we chat aboit this? skype? id: jlpedrosa

jlpedrosa · 2016-04-28T16:04:51Z

Is it clear now why the dynamic is mandatory? There are no style check warnings, existing tests and new ones are passing.
Ok now? I'm available for discussing/explaining.

Rgds

JL

trohovsky · 2016-04-28T19:10:40Z

You intention for using "dynamic" in URI path is clear form beginning, but as I said, it is not good to create such a convention. What if somebody wants to name the job "dynamic" (or already named) and actually don't select the job by the new header?

jlpedrosa · 2016-04-28T19:35:20Z

That point you said of someone using the job name "dynamic" is the only
downside. What I am trying to achieve is being backwards compatible.

We could do something more elaborated like:
Springbatch:dynamic
Springbatch:static:jobName
But this would break backwards compatibility, if you want we can make the
keyword "use_message_header" so it's "absolutely" (the fake job sugested by
Claus) impossible that anyone wants to use it.

Due to the sytax uri constrains the path must be set, even if (like in this
case) needs to be set.

So options)

use dynamic (or more elaborated keyword)
break backwards compatibility
send patch to trash
any other idea?

I think 1) is the way forward.
On Apr 28, 2016 9:10 PM, "Tomas Rohovsky" notifications@github.com wrote:

You intention for using "dynamic" in URI path is clear form beginning, but
as I said, it is not good to create such a convention. What if somebody
wants to name the job "dynamic" (or already named) and actually don't
select the job by the new header?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#968 (comment)

trohovsky · 2016-04-28T20:09:31Z

Does a use case, that no job is known from beginning, really exist? I don't think so. If yes, then recipientList or toD are still in the game.

jlpedrosa · 2016-04-28T20:30:38Z

Hi Tomas

I understand you think drop the patch is best and use tod.

In my opinion:
Force to have a know job is more complicated than having a dynamic tag.

In my use case I don't know any job during the route creation, as I'm
mixing this with kafka to create a generic kafka interface for a job
launcher.

And I doubt anyone would try to create a job with name "dynamic" or
"use_header_jobname".

As I said in the Jira ticket, toD would work for me, but as Claus pointed
out it would create unncesaary amount of endpoints/producers.

Regards
On Apr 28, 2016 22:09, "Tomas Rohovsky" notifications@github.com wrote:

Does a use case, that no job is known from beginning, really exist? I
don't think so. If yes, then recipientList or toD are still in the game.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#968 (comment)

trohovsky · 2016-04-29T08:28:15Z

I am not for rejecting the patch at all, I just don't like using a constant path to achieve a no-job endpoint during startup.

jlpedrosa · 2016-04-29T08:41:41Z

Tomas,

I agree with you, having a magic keyword, it's not a perfect solution, but
I can't see any other way without breaking backwards compatibility.

So options)

use dynamic or more elaborated "keyword" as path

Not perfect from cleannes poing of view
Keeps backwards compatibility.
Keeps single endpoint

Use a more sofisticated path like springbatch:(dynamic|static:jobName)

Clean solution
Break backwards compatibility
Keeps single endpoint

Don't do the dynamic header jobname and use tod (so, send patch to trash)

Clean solution
keeps backwards compatibility
mutliple endpoint

don't use dynamic or more elaborated "keyword" as path, and force to
have an existing job at compilation

Not perfect from cleannes poing of view (force to have a job is not
clean)
Keeps backwards compatibility.
Keeps single endpoint

any other idea?

maybe the answer is mixed? 2) for 2.18 and 1) for 2.17?

Tomas, Claus is your call.

On Fri, Apr 29, 2016 at 10:28 AM, Tomas Rohovsky notifications@github.com
wrote:

I am not for rejecting the patch at all, I just don't like using any
constant path to achieve a no-job endpoint during startup.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#968 (comment)

JustinKyleHixson · 2016-04-29T20:45:26Z

...ing-batch/src/test/java/org/apache/camel/component/spring/batch/SpringBatchEndpointTest.java

+    }
+
+    @Test
+    public void dynamicJobWorksIfHeaderWithInvalidJobName() throws Exception {


Shouldn't this test be named dynamicJobFailsIfHeaderWithInvalidJobName with the word Fails replacing the word Works ?

davsclaus · 2016-05-07T08:00:32Z

We can add a query option jobFromHeader you can set to true,then it will ignore the job name you set in the context path.

jlpedrosa · 2016-05-07T08:04:12Z

Ok, that´s ok, I’ll implement it on monday, is it enough?

On 07 May 2016, at 10:00, Claus Ibsen notifications@github.com wrote:

We can add a query option jobFromHeader you can set to true,then it will ignore the job name you set in the context path.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub #968 (comment)

davsclaus · 2016-05-13T05:45:39Z

Yeah sounds good. Maybe open a new PR with the jobFromHeader so its easier to review

davsclaus · 2016-05-21T05:46:16Z

@jlpedrosa are you able to work on a new PR?

jlpedrosa · 2016-05-21T07:14:41Z

Hi Claus

Yes, I'll get it done next week, is it ok?
On May 21, 2016 7:46 AM, "Claus Ibsen" notifications@github.com wrote:

@jlpedrosa https://github.com/jlpedrosa are you able to work on a new
PR?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#968 (comment)

davsclaus · 2016-05-21T07:17:36Z

Yes thats fine

…Name in the headers, Test passed, code rules passed.

jlpedrosa · 2016-05-27T11:02:34Z

Contiunued in
#1001

as a sugestion of @davsclaus

Entesb 9237 2.23.x

patch to support dynamic job execution based on message headers for s…

d06d850

…prinbatch component https://issues.apache.org/jira/browse/CAMEL-9733

trohovsky reviewed Apr 28, 2016
View reviewed changes

fixed source check style miss alingments

61c12ea

trohovsky reviewed Apr 28, 2016
View reviewed changes

Joseluis Pedrosa added 2 commits April 28, 2016 17:15

Refactored variable name, and removed a redudant block of code.

5b2ba1a

compare moved to equals and NullPointer safe

ef2a4b5

JustinKyleHixson reviewed Apr 29, 2016
View reviewed changes

Joseluis Pedrosa added 2 commits May 27, 2016 12:55

Added extra parameter to SpringBatch connector to support dynamic job…

65184f9

…Name in the headers, Test passed, code rules passed.

missed file in commit.

e995c0f

jlpedrosa closed this May 27, 2016

jlpedrosa mentioned this pull request May 27, 2016

Camel 9733 Add support for JobName specified in the message. #1001

Closed

zregvart pushed a commit to zregvart/camel that referenced this pull request Nov 8, 2019

Merge pull request apache#968 from jboss-fuse/ENTESB-9237-2.23.x

5177b6e

Entesb 9237 2.23.x

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for dynamic spring batch job invoication based on message header #968

Added support for dynamic spring batch job invoication based on message header #968

jlpedrosa commented Apr 28, 2016

davsclaus commented Apr 28, 2016

trohovsky Apr 28, 2016

jlpedrosa Apr 28, 2016

davsclaus commented Apr 28, 2016

trohovsky Apr 28, 2016

jlpedrosa Apr 28, 2016

jlpedrosa commented Apr 28, 2016

trohovsky Apr 28, 2016

jlpedrosa commented Apr 28, 2016

jlpedrosa commented Apr 28, 2016

trohovsky commented Apr 28, 2016

jlpedrosa commented Apr 28, 2016

trohovsky commented Apr 28, 2016

jlpedrosa commented Apr 28, 2016

trohovsky commented Apr 29, 2016 •

edited

jlpedrosa commented Apr 29, 2016 •

edited

JustinKyleHixson Apr 29, 2016

davsclaus commented May 7, 2016

jlpedrosa commented May 7, 2016

davsclaus commented May 13, 2016

davsclaus commented May 21, 2016

jlpedrosa commented May 21, 2016

davsclaus commented May 21, 2016

jlpedrosa commented May 27, 2016

Added support for dynamic spring batch job invoication based on message header #968

Added support for dynamic spring batch job invoication based on message header #968

Conversation

jlpedrosa commented Apr 28, 2016

davsclaus commented Apr 28, 2016

trohovsky Apr 28, 2016

Choose a reason for hiding this comment

jlpedrosa Apr 28, 2016

Choose a reason for hiding this comment

davsclaus commented Apr 28, 2016

trohovsky Apr 28, 2016

Choose a reason for hiding this comment

jlpedrosa Apr 28, 2016

Choose a reason for hiding this comment

jlpedrosa commented Apr 28, 2016

trohovsky Apr 28, 2016

Choose a reason for hiding this comment

jlpedrosa commented Apr 28, 2016

jlpedrosa commented Apr 28, 2016

trohovsky commented Apr 28, 2016

jlpedrosa commented Apr 28, 2016

trohovsky commented Apr 28, 2016

jlpedrosa commented Apr 28, 2016

trohovsky commented Apr 29, 2016 • edited

jlpedrosa commented Apr 29, 2016 • edited

JustinKyleHixson Apr 29, 2016

Choose a reason for hiding this comment

davsclaus commented May 7, 2016

jlpedrosa commented May 7, 2016

davsclaus commented May 13, 2016

davsclaus commented May 21, 2016

jlpedrosa commented May 21, 2016

davsclaus commented May 21, 2016

jlpedrosa commented May 27, 2016

trohovsky commented Apr 29, 2016 •

edited

jlpedrosa commented Apr 29, 2016 •

edited