Cron Support for CDC #59
Conversation
component/pom.xml
Outdated
| @@ -585,6 +585,7 @@ | |||
| io.siddhi.query.*;version="${siddhi.import.version.range}", | |||
| *;resolution:=optional | |||
| </Import-Package> | |||
| <DynamicImport-Package>*</DynamicImport-Package> | |||
There was a problem hiding this comment.
Avoid using dynamic imports. Add the necessary packages in the import-packages section
| ), | ||
| @Parameter( | ||
| name = "cron.expression", | ||
| description = "This is used to specify a timestamp in cron expression. " + |
There was a problem hiding this comment.
I think the description is wrong. Please check
| // Tell quartz to schedule the job using our trigger | ||
| scheduler.scheduleJob(cron, trigger); | ||
| } catch (SchedulerException e) { | ||
| log.error("The error occurs at scheduler start in SiddhiApp : " + e); |
There was a problem hiding this comment.
Shall we change this as "An error occurred when scheduling job in siddhi app for cdc polling"
| @@ -185,6 +190,9 @@ public void resume() { | |||
| public void run() { | |||
| try { | |||
| pollingStrategy.poll(); | |||
There was a problem hiding this comment.
Shouldn't this come in the else part? In this scenario, whenever the cdc source starts running, a poll() happens right. Please confirm
There was a problem hiding this comment.
poll() is called to set the lastReadPollingColumnValue. If the poll() is called in the scheduleJob() then records which has been inserted between the time app starts running and the first cronschedule will be missed. To ignore this issue the lastReadPollingColumnValue has to be stored. So it shouldn't come in the else part.
| } | ||
| } catch (SQLException ex) { | ||
| throw new CDCPollingModeException(buildError("Error in polling for changes on %s.", tableName), ex); |
There was a problem hiding this comment.
Any reason to log the error without throwing?
There was a problem hiding this comment.
Followed the same approach.
| } finally { | ||
| CDCPollingUtil.cleanupConnection(resultSet, statement, connection); |
There was a problem hiding this comment.
We should close connection and statement after using. And re-initialize when using again.
There was a problem hiding this comment.
Connection and statement are closed at the end of the method.
|
|
||
| // Constants for Cron Support | ||
| public static final String CRON_EXPRESSION = "cron.expression"; | ||
| public static final String JOB_GROUP = "JobGroup"; |
There was a problem hiding this comment.
Shall we add some meaningful names if those are getting printed when debugging or on errors?? Eg: CDCPollingGroup
| @@ -92,7 +97,7 @@ public CDCPoller(String url, String username, String password, String tableName, | |||
| } else { | |||
| log.debug(DefaultPollingStrategy.class + " is selected as the polling strategy."); | |||
There was a problem hiding this comment.
Shall we validate and throw app creation error if cron and some other strategy is selected?
Purpose
Security checks
Related Issues
wso2/streaming-integrator#115
#58