[jira] [Commented] (FALCON-485) Simplify JMS Message Sender/Consumer and use Workflow Context

["Venkatesh Seetharam (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/FALCON-485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14048818#comment-14048818 ] 

Venkatesh Seetharam commented on FALCON-485:
--------------------------------------------

Thanks [~shaik.idris] for taking time to review.

bq. Is this change backward compatible, with just Falcon Server upgrade, how does message flow happens to/from the scheduled "post-processing" actions from/to Falcon as the context file will be missing?
This is probably not backwards compatible. Post falcon upgrade, existing scheduled pipelines will not write context to HDFS. They need to be rescheduled. Is this a hard requirement? 

bq. Looks like a bug. For now I have proceeded the review/testing by changing the code.
Pls apply further patches as this has been fixed. 

bq. Actually enum already has a method name() and getName() is adding more confusion to it. 
Agree, but this is how it was earlier and a getter is more intuitive no?

bq. Just realised the other ProcessProducerTest is failing now.
Apply further patches and all tests pass. They are fixed.

bq. Can you please look at WorkflowExecutionContext.create() and its behaviour with all Messaging testcases.
bq. Looks like JSON serializer/deserilizer is treating the ENUM has Map<String,String> instead of Map<WorkflowArg,String>
This is all fixed. Please apply all patches in FALCON-327

bq. I think this is FalconTopicSubscriber is renamed and moved to Messaging, the idea of keeping messaging separate was the jar was shipped with scheduled entity, but FalconTopicSubscriber is actually part of Falcon server and listens to topic.
Logically yes but does it hurt if its shipped?

bq. WorkflowJobEndNotificationService.instrumentAlert() needs WorkflowEngineFactory.getWorkflowEngine(), this dependency is missing in pom.xml
Hmmm, since this is executed in webapp, this seems to be working. Logically, this should be moved to webapp module.

bq. Can we add Assert.Fail by adding try/catch in JMSMessageConsumerTest.testSubscriber()'s Exception block. All the exceptions in the thread are ignored.
This is just a rename of the test and this is how it was, no? Also, if there is an exception, TestNG would intercept and fail the test.

bq. JMSMessageProducer.FALCON_FILTER needs atleast these many params, which are further used for instrumentation and retries/reruns. run-id etc are missing.
Please look at other patches and you'll see how this is handled. WorkflowExecutionContext is decorated with these for late framework.

> Simplify JMS Message Sender/Consumer and use Workflow Context
> -------------------------------------------------------------
>
>                 Key: FALCON-485
>                 URL: https://issues.apache.org/jira/browse/FALCON-485
>             Project: Falcon
>          Issue Type: Sub-task
>          Components: messaging
>    Affects Versions: 0.6
>            Reporter: Venkatesh Seetharam
>            Assignee: Venkatesh Seetharam
>              Labels: refactoring
>             Fix For: 0.6
>
>         Attachments: FALCON-485.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.2#6252)