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

SWITCHYARD-2433 Use consumer.delay and consumer.initialDelay instead of ... #742

Closed
wants to merge 1 commit into from

Conversation

dvirgiln
Copy link
Contributor

@trohovsky
Copy link
Contributor

Hay David.

I haven't tested your patch, but this line seems to me like duplicate: https://github.com/dvirgiln/components/blob/SWITCHYARD-2433/camel/camel-sql/src/main/java/org/switchyard/component/camel/sql/deploy/CamelSqlInboundHandler.java#L59. Can't be used the createRouteDefinition method from the parent (InboundHandler#createRouteDefinition) insead of overriding it? If the duplicate line I've pointed out will be removed, the only difference would be the condition for presence of the period attribute which I believe can be optional.

Tomas

@dvirgiln
Copy link
Contributor Author

Hi Tomas,
I tested this PR using the sql camel quickstart and works perfectly.

I realized about the line that is also in the parent class. I tried removing this line in the CamelSqlINboundHandler and I receive an exception:

https://gist.github.com/dvirgiln/f983b02f77d709a16587

Also I thought about removing completely this specific Handler, because now maybe had no sense to have this handler as the timer is not necesary to be configured. I removed the handler, also the activator. The result of executing the test was not sucessful. It ended up in an infinite loop waiting for messages.

@igarashitm
Copy link
Member

Hi @dvirgiln ,

I looked at the code, I believe we can remove CamelSqlInboundHandler as it now doesn't do any sql binding specific job (assuming periodAttributeMandatory check is no longer needed). Not sure what's happening on your test failure, but it's worth to debug it.

@dvirgiln
Copy link
Contributor Author

Hi, I tried the solution removing the CamelSqlInboundHandler and was not working. It was the first thing I tried. What was happening was that the SQLProducer statement was the INSERT statement instead of the SELECT.

When you execute this code of the PR, or the current master one the SQL Producer statement is the SELECT.

What i am pretty sure is that this code works, and removing the handler and using the common one is not.

@kcbabo
Copy link
Member

kcbabo commented Nov 26, 2014

I added some comments to the JIRA.

@dvirgiln
Copy link
Contributor Author

dvirgiln commented Dec 3, 2014

As Tom suggested me, maybe is good to create a org.switchyard.component.camel.sql.model.v2 package, and include the changes in the model directly in the V2CamelSqlBindingModel.

I have done this development, but before commiting and removing the current changes I would like this to be confirmed.

In my local machine I've included a V2CamelSqlBindingModel that extends from the V1CamelSqlBindingModel

@kcbabo
Copy link
Member

kcbabo commented Dec 4, 2014

+1 on V2 model.

@dvirgiln
Copy link
Contributor Author

dvirgiln commented Dec 4, 2014

I am taking a look why the output of the SQL Camel Component is not redirected to the Service Implementation of Switchyard. I checked the SwitchyardProducer, the value of switchyardExchange.getContract().getProviderOperation().getName() is Null.

Also checked in the version of the master, that is working with the timer, this value contains "consume"

@kcbabo
Copy link
Member

kcbabo commented Dec 4, 2014

The provider operation is set after the SY exchange is sent, so if you are looking at the value returned by that method before switchyardExchange.send() is called, then I would expect it to be null.

Have you enabled message tracing in SY to see what's getting passed along the bus? You can do that via a domain property in switchyard.xml, e.g.

<domain>
      <properties>
        <property name="org.switchyard.handlers.messageTrace.enabled" value="true"/>
      </properties>
  </domain>

@dvirgiln
Copy link
Contributor Author

dvirgiln commented Dec 4, 2014

Taking a look to the logs

@rcernich
Copy link
Member

rcernich commented Dec 8, 2014

pushed

@rcernich rcernich closed this Dec 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants