abdullah alamoudi has posted comments on this change. Change subject: Refactor General Active Classes ......................................................................
Patch Set 1: (21 comments) https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveEvent.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveEvent.java: Line 28: private final EntityId feedId; > Why does this have a feedId? Is there a better name? Done https://asterix-gerrit.ics.uci.edu/#/c/977/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: Line 33: public static final ActiveJobNotificationHandler INSTANCE = new ActiveJobNotificationHandler(); > Should this really be global? Not sure if this is about the String or the INSTANCE. I think the instance should be global since it is a singleton that can be easily accessed by anyone who wants to listen for events that are related to active jobs. What would the alternative look like? As for the String, we use it to basically tag active JobSpecifications and so I think it should be available to all and written in a single place. Am I missing something? Line 50: while (true) { > Do we ever leave this loop? Done https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveManager.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveManager.java: Line 27: * An implementation of the IFeedManager interface. > That doesn't seem to be true ... Done https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntimeId.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntimeId.java: Line 53: return (other.entityId.equals(entityId) && other.getFeedRuntimeType().equals(runtimeId) > MAJOR SonarQube violation: Done https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntimeManager.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntimeManager.java: Line 33: private final Map<ActiveRuntimeId, ActiveRuntime> feedRuntimes; > activeRuntimes? Done https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntimeRegistry.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntimeRegistry.java: Line 27: * An implementation of the IFeedManager interface. > IFeedManager? Done Line 43: public void deregisterRuntime(ActiveRuntimeId feedId) { > feedId? Done Line 51: } catch (Exception e) { > CRITICAL SonarQube violation: Done Line 60: public synchronized void registerRuntime(ActiveRuntime feedRuntime) { > feedRuntime? Done Line 70: public ActiveRuntime getRuntime(ActiveRuntimeId feedRuntimeId) { > feedRuntimeId? Done https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/IActiveJobBuilder.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/IActiveJobBuilder.java: Line 21: public interface IActiveJobBuilder { > It seems that this is unused. Could we remove it? Done. I wanted to use it for some more extensibility. I guess when I introduce that extensibility, I will recreate this. https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ActiveLifecycleListener.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ActiveLifecycleListener.java: Line 45: public static final ActiveLifecycleListener INSTANCE = new ActiveLifecycleListener(); > Should this really be global? This has always been a global singleton. Am I missing something here? Line 53: this.executorService.execute(ActiveJobNotificationHandler.INSTANCE); > Why are some members prefixed by "this."? Not sure about this!!!. Will remove it. This class was just renamed but for some reason gerrit thinks it is a new class Line 79: public void recieve(ActivePartitionMessage message) { > s/recieve/receive/ Done https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/aql/translator/QueryTranslator.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/aql/translator/QueryTranslator.java: Line 271: Thread.currentThread().setName(QueryTranslator.class.getSimpleName()); > What is this thread renaming used for? I think that we need at least a good Done Line 285: case SET: { > Remove the curly braces for each case? Done Line 736: StringBuilder builder = null; > It seems that `builder == null` is equivalent with `resourceInUse == false` Done https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/management/FeedCollectInfo.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/management/FeedCollectInfo.java: Line 30: public EntityId sourceFeedId; > MAJOR SonarQube violation: Done https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/management/FeedEventsListener.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/management/FeedEventsListener.java: Line 590: public void deregisterFeedEventSubscriber(FeedConnectionId connectionId, IFeedLifecycleEventSubscriber subscriber) { > Why do we register with an Entity and de-register with a FeedConnectionId? Ouch. This is a historical leftover from when we had feeds as the sole active entities. But since the listener is associated with a single feed, they are not needed anymore. I removed them Line 628: public synchronized boolean isConnectedToDataset(String dataverseName, String datasetName) { > This is a little scary - we'll say that we're connected even though the dat Done. When this is called, we know the dataverses match :|| -- To view, visit https://asterix-gerrit.ics.uci.edu/977 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0a8f33ee5b45c5e090b08c24a102e369aae43c04 Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
