abdullah alamoudi has posted comments on this change. Change subject: Enable Feed Changes to work with BAD project ......................................................................
Patch Set 1: (3 comments) Let's think of the case where there are no listeners, Should the job be removed? What if it is a repeatable job? can the job be registered another time? The only reason we register that job through the job lifecycle listener is that we don't have the job Id yet when we have the active entity and listener https://asterix-gerrit.ics.uci.edu/#/c/1524/1/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveJobNotificationHandler.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveJobNotificationHandler.java: PS1, Line 56: er listener = e what if there was no listener. this will cause a null pointer exception PS1, Line 60: if (event.getEventKind() == Kind.JOB_FINISHED && !listener.isRepeatable() Let's remove the block completely and the listener can remove itself when notified PS1, Line 75: removeJob maybe rename to remove job and listener? make synchronized and then call unregisterListener from inside the method. also make the two maps concurrent: this.jobId2ActiveJobInfos = new HashMap<>(); this.entityEventListeners = new HashMap<>() Can you also create a JIRA issue to create unit tests for this class? seems too important to get right and doesn't have unit tests that cover corner cases -- To view, visit https://asterix-gerrit.ics.uci.edu/1524 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib62184b67aff564475ef9b58790ff96409195b77 Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Steven Jacobs <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
