Till Westmann has posted comments on this change. Change subject: Refactor General Active Classes ......................................................................
Patch Set 1: (20 comments) I've got a few comments and questions. Also, it seems that a number of the SQ comments regarding exceptions, unused parameters, and formatting are quite valid :) 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? 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? Line 50: while (true) { Do we ever leave this loop? 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 ... 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? 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? Line 43: public void deregisterRuntime(ActiveRuntimeId feedId) { feedId? Line 60: public synchronized void registerRuntime(ActiveRuntime feedRuntime) { feedRuntime? Line 70: public ActiveRuntime getRuntime(ActiveRuntimeId feedRuntimeId) { feedRuntimeId? 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? https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/AsterixAppRuntimeContext.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/AsterixAppRuntimeContext.java: Line 122: private ActiveManager feedManager; Should this be an active manager? 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? Line 53: this.executorService.execute(ActiveJobNotificationHandler.INSTANCE); Why are some members prefixed by "this."? Line 79: public void recieve(ActivePartitionMessage message) { s/recieve/receive/ 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 comment why this is useful. Line 285: case SET: { Remove the curly braces for each case? Line 736: StringBuilder builder = null; It seems that `builder == null` is equivalent with `resourceInUse == false`. Can we remove one? 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? And why are both not used? Line 628: public synchronized boolean isConnectedToDataset(String dataverseName, String datasetName) { > MAJOR SonarQube violation: This is a little scary - we'll say that we're connected even though the dataverse doesn't match? https://asterix-gerrit.ics.uci.edu/#/c/977/1/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedMetadataUtil.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedMetadataUtil.java: Line 156: operandId = null; Shouldn't operandId just be declared here? -- 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-HasComments: Yes
