abdullah alamoudi has posted comments on this change. Change subject: Cleanup Feed CodeBase ......................................................................
Patch Set 3: (57 comments) more comments https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/FeedServletUtil.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/FeedServletUtil.java: Line 29: File an issue to restore feed web console https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/FeedLifecycleListener.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/FeedLifecycleListener.java: Line 156: return null; return empty set. and fix caller not to check for null Line 160: public Set<IClusterManagementWork> notifyNodeJoin(String joinedNodeId) { same as before Line 165: public void notifyRequestCompletion(IClusterManagementWorkResponse response) { stopped using Line 169: public void notifyStateChange(ClusterState previousState, ClusterState newState) { we don't use this anymore https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/FeedOperations.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/FeedOperations.java: Line 138: public static Pair<IOperatorDescriptor, AlgebricksPartitionConstraint> buildDiscontinueFeedMessengerRuntime( remove unused https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/FeedWorkCollection.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/FeedWorkCollection.java: Line 82: PrintWriter writer = new PrintWriter(System.out, true); write to a string and log the output Line 116: ((SubscribeFeedWork) work).request.setSubscriptionStatus(ConnectionStatus.ACTIVE); wrong level https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/AsterixLSMInsertDeleteOperatorNodePushable.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/AsterixLSMInsertDeleteOperatorNodePushable.java: Line 120: th.printStackTrace(); remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/pom.xml File asterixdb/asterix-external-data/pom.xml: Line 197: <groupId>org.apache.hyracks</groupId> scope test Line 303: <groupId>org.mockito</groupId> scope test Line 308: <groupId>org.powermock</groupId> file and issue for license and notice for powermock https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IExternalIndexer.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IExternalIndexer.java: Line 29: @SuppressWarnings("deprecation") remove it https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IRecordWithMetaDataAndPKParser.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IRecordWithMetaDataAndPKParser.java: Line 27: @SuppressWarnings("deprecation") remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IRecordWithPKDataParser.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IRecordWithPKDataParser.java: Line 25: @SuppressWarnings("deprecation") remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/ITupleForwarder.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/ITupleForwarder.java: Line 27: public interface ITupleForwarder { remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/AbstractFeedDataFlowController.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/AbstractFeedDataFlowController.java: Line 29: @SuppressWarnings("deprecation") remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/ChangeFeedDataFlowController.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/ChangeFeedDataFlowController.java: Line 31: @SuppressWarnings("deprecation") remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/ChangeFeedWithMetaDataFlowController.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/ChangeFeedWithMetaDataFlowController.java: Line 32: public class ChangeFeedWithMetaDataFlowController<T, O> extends FeedWithMetaDataFlowController<T, O> { remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/CounterTimerTupleForwarder.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/CounterTimerTupleForwarder.java: Line 37: @SuppressWarnings("deprecation") remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedTupleForwarder.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedTupleForwarder.java: Line 110: public void close() throws HyracksDataException { Fix this using finally https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/api/IFeedRuntime.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/api/IFeedRuntime.java: Line 39: DISCARD // Memory budget has been consumed. Disk space budget has been consumed. Now we're discarding too long https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/DistributeFeedFrameWriter.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/DistributeFeedFrameWriter.java: Line 75: * @throws HyracksDataException WS https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedExceptionHandler.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedExceptionHandler.java: Line 78: //TODO: Fix logging of exceptions log using the logger for now https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedFrameCollector.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedFrameCollector.java: Line 84: public void setFrameWriter(IFrameWriter nextOnlyWriter) { change the name Line 114: th.printStackTrace(); remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedFrameSpiller.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedFrameSpiller.java: Line 42: * cannot process incoming data at its arrival rate. The maximum size of data (tuples) that can be spilled to disk is configured using the property reformat the comment to fit 120 char width Line 68: this.connectionId = connectionId; make generic and not feed related https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java: Line 109: try { join thread then close writer Line 126: try { > synchronized consumer.cause() just make it volatile https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FrameDistributor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FrameDistributor.java: Line 63: collector.nextFrame(frame); Fix. What should be done?: 0. mark failure so no one can subscribe or unsubscribe. 1. Throw the throwable. 2. when fail() is called, call fail on all subscribers 3. close all the subscribers. Line 88: try { handle failure as in nextFrame() Then add unit tests for this frame writer https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/SyncFeedRuntimeInputHandler.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/SyncFeedRuntimeInputHandler.java: Line 51: throw new HyracksDataException(th); change from throwable to hde in the catch. not just here. https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/management/FeedMemoryManager.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/management/FeedMemoryManager.java: Line 34: private static final String ERROR_INVALID_FRAME_SIZE = "The size should be an integral multiple of the default frame size"; long line Line 41: private int handed; handedOut Line 53: if (handed + 1 <= budget) { handed < budget Line 68: int multiples = bufferSize / defaultFrameSize; rename to multiplier Line 73: largeFramesPool = new ArrayDeque<>(); only create the pool when you want to release. do some refactoring Line 93: private Line 124: } catch (Throwable th) { remove try catch Line 176: // check subscribers loop until you can't satisfy request add test cases for subscriptions Line 209: // none of the above, add to subscribers and return true WSs https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/CollectionRuntime.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/CollectionRuntime.java: Line 55: if (!(isCollectionOver())) { change to interrupted exception https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/FeedRuntimeId.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/FeedRuntimeId.java: Line 30: public static final String DEFAULT_OPERAND_ID = "N/A"; remove dataset. add comment as to what is operand. then fix constructor https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/IngestionRuntime.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/IngestionRuntime.java: Line 47: dWriter.subscribe(collector); only allocate on first subscriber Line 50: adapterRuntimeManager.start(); only start on first Line 59: adapterRuntimeManager.stop(); only stop and cleanup on the last one Line 63: public void endOfFeed() throws HyracksDataException { remove Line 71: public void terminate() { fix terminate not to call unsubscribe before stopping the manager https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/SubscribableRuntime.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/SubscribableRuntime.java: Line 56: public synchronized void subscribe(CollectionRuntime collectionRuntime) throws HyracksDataException { This is never called. Should we remove? Line 63: public synchronized void unsubscribe(CollectionRuntime collectionRuntime) throws HyracksDataException { also unsubscribe is never called https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedIntakeOperatorDescriptor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedIntakeOperatorDescriptor.java: Line 107: private IAdapterFactory createExtenralAdapterFactory(IHyracksTaskContext ctx, int partition) throws Exception { typo https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedIntakeOperatorNodePushable.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedIntakeOperatorNodePushable.java: Line 73: Thread.currentThread().setName("Intake Thread"); remove renaming of thread Line 118: } remove https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedMetaStoreNodePushable.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedMetaStoreNodePushable.java: Line 156: FeedUtils.processFeedMessage(buffer, message, fta); ensure consistency between feed operators' members names. try consolidate both compute and store https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-external-data/src/test/java/org/apache/asterix/external/feed/test/FeedSpillerUnitTest.java File asterixdb/asterix-external-data/src/test/java/org/apache/asterix/external/feed/test/FeedSpillerUnitTest.java: Line 96: * Check that we have 3 files. WSs https://asterix-gerrit.ics.uci.edu/#/c/798/3/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java: Line 2214: @SuppressWarnings("rawtypes") remove -- To view, visit https://asterix-gerrit.ics.uci.edu/798 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I545bc4f8560564e4c868a80d27c77a4edd97a8b8 Gerrit-PatchSet: 3 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Murtadha Hubail <hubail...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
