abdullah alamoudi has posted comments on this change. Change subject: Added Channels to Asterix ......................................................................
Patch Set 7: (73 comments) Done with first round :) https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/pom.xml File asterix-active/pom.xml: Line 29: <artifactId>asterix-active-jobs</artifactId> change to asterix-active just as the directory name. otherwise, this gets confusing when you open it in eclipse! Line 37: <dependency> WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/ActiveActivity.java File asterix-active/src/main/java/org/apache/asterix/active/ActiveActivity.java: Line 51: if (!(other instanceof FeedActivity)) { this checks for instance of FeedActivity and then cast to ActiveActivity!! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/ActiveJobId.java File asterix-active/src/main/java/org/apache/asterix/active/ActiveJobId.java: Line 24: Clearly this class is not needed since it is simply a wrapper for ActiveEntityId. What am I missing? https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/api/IActiveMessage.java File asterix-active/src/main/java/org/apache/asterix/active/api/IActiveMessage.java: Line 24: Note: Steven, I don't need this at all and in my opinion, this is a bad way to do messaging. I will be using message brokers for all my message needs. I suggest you try and move away from this as well. Not necessarily right now but at should be some time soon. https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/channel/ChannelRuntime.java File asterix-active/src/main/java/org/apache/asterix/active/channel/ChannelRuntime.java: Line 33: So this is a hyracks job which gets run based on a frequency? Why are we calling hyracks from multiple NCs? why not have the timer on the CC? https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/channel/StoredJobRuntime.java File asterix-active/src/main/java/org/apache/asterix/active/channel/StoredJobRuntime.java: Line 51: activeManager.getChannelJobService().runChannelJob(jobSpec); this runChannelJob will communicate with the CC and run the job through hyracks. why do we need the runtime to reside in multiple NCs? https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/operators/ActiveMessageOperatorDescriptor.java File asterix-active/src/main/java/org/apache/asterix/active/operators/ActiveMessageOperatorDescriptor.java: Line 29: I think this needs to go away. you can keep it for now, but I will move feeds away from using it and instead use the message broker. https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/operators/ActiveMessageOperatorNodePushable.java File asterix-active/src/main/java/org/apache/asterix/active/operators/ActiveMessageOperatorNodePushable.java: Line 69: * WS. conflict resolution gone BAD! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/operators/ActiveMetaNodePushable.java File asterix-active/src/main/java/org/apache/asterix/active/operators/ActiveMetaNodePushable.java: Line 82: /** The (singleton) instance of IFeedManager **/ comments don't reflect the new change. https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/operators/ChannelMetaOperatorDescriptor.java File asterix-active/src/main/java/org/apache/asterix/active/operators/ChannelMetaOperatorDescriptor.java: Line 6: * WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/active/operators/RepetitiveChannelOperatorNodePushable.java File asterix-active/src/main/java/org/apache/asterix/active/operators/RepetitiveChannelOperatorNodePushable.java: Line 88: what is the writer here typically? https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/external/feed/api/IActiveManager.java File asterix-active/src/main/java/org/apache/asterix/external/feed/api/IActiveManager.java: Line 32: * WS! Line 40: * WS! Line 48: * WS! Line 56: * WS! Line 64: * WS! Line 72: * WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-active/src/main/java/org/apache/asterix/external/feed/api/IAdapterRuntimeManager.java File asterix-active/src/main/java/org/apache/asterix/external/feed/api/IAdapterRuntimeManager.java: Line 49: * WS! Line 56: * WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/ReplaceableSinkOperator.java File asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/ReplaceableSinkOperator.java: Line 6: * WS! Line 23: This is only called from a single location where pipelineEnd is always set to false. Why not add a final boolean field isPipeLineEnd in the SinkOperator https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/CommitRuntime.java File asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/CommitRuntime.java: Line 142: flushIfNotFailed(); There is a theoretical chance that writer.close() is called without writer.open() having been called. We need every single operator to conform to the iframewriter interface. might also add a unit test case for this one to ensure that it always conform? Look at the test case for the BtreeSearchOperatorNodePushable. org.apache.hyracks.storage.am.btree.test.FrameWriterTest. https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/ReplaceSinkOpWithCommitOpRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/ReplaceSinkOpWithCommitOpRule.java: Line 68: } if we follow the suggestion I proposed (removing replacableSinkOperator), the code path becomes cleaner with no branching due to instanceof check. https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-algebra/src/main/java/org/apache/asterix/translator/CompiledStatements.java File asterix-algebra/src/main/java/org/apache/asterix/translator/CompiledStatements.java: Line 298: private final boolean returnRecord; Can you explain what are the options for the return values are? 1. a field? 2. set of fields? 3. the whole record(s)? https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java File asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: Line 383: insertOp.setAdditionalFilteringExpressions(additionalFilteringExpressions); instead of casting stmt multiple times? why not define another variable of type CompiledInsertStatement? Line 407: Mutable<ILogicalExpression> nameRef = new MutableObject<ILogicalExpression>( this will not work for nested fields Line 424: The last two lines of the if and else blocks can be refactored out? Line 434: IAType outputRecordType = metadataProvider.findOutputRecordType(); I think this is not the right way to get the output record type. why not get it from the dataset in the insert statement? what if output record type == null? how does it proceed? https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/main/java/org/apache/asterix/app/external/ActiveJobNotificationHandler.java File asterix-app/src/main/java/org/apache/asterix/app/external/ActiveJobNotificationHandler.java: Line 25: import java.util.HashMap; This class is becoming painful to look at. Or it has been that way since the beginning. I think we will refactor it some time soon. https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/main/java/org/apache/asterix/app/external/ActiveLifecycleListener.java File asterix-app/src/main/java/org/apache/asterix/app/external/ActiveLifecycleListener.java: Line 277: private final Map<IFeedJoint, List<String>> recoverableIntakeFeedIds; WSs! Line 383: public void run() { WSs! Line 426: WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/main/java/org/apache/asterix/app/external/ActiveWorkRequestResponseHandler.java File asterix-app/src/main/java/org/apache/asterix/app/external/ActiveWorkRequestResponseHandler.java: Line 157: } Can you please add a TODO to get rid of all the thread.sleep calls. I think they are almost always a very bad practice. https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/main/java/org/apache/asterix/app/external/ChannelJobService.java File asterix-app/src/main/java/org/apache/asterix/app/external/ChannelJobService.java: Line 74: } please, ensure Metadata transactions are always either committed or aborted. Maybe even add a unit test case to make sure this component always respects that. Line 95: URL url = new URL(targetURL); Why do we go to brokers through http? I think we should change this to use nc to nc messaging when it is merged. Line 108: wr.writeBytes(urlParameters); put wr in a try with clause to ensure its closure. https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/main/java/org/apache/asterix/app/external/ChannelOperations.java File asterix-app/src/main/java/org/apache/asterix/app/external/ChannelOperations.java: Line 6: * WS! Line 43: * WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/main/java/org/apache/asterix/app/external/FeedOperations.java File asterix-app/src/main/java/org/apache/asterix/app/external/FeedOperations.java: Line 67: * WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/main/java/org/apache/asterix/aql/translator/QueryTranslator.java File asterix-app/src/main/java/org/apache/asterix/aql/translator/QueryTranslator.java: Line 371: case UPSERT: { instead of having the check here, remove it and move the block to the insert case: with fall through i.e, no break at the end. Line 375: metadataProvider.setResultAsyncMode(resultDelivery == ResultDelivery.ASYNC why is it always async? Line 2532: boolean metaDataLock = true; metaDataLock variable is not needed since it will always be true Line 2572: Why not get rid of the broker completely since the way I am seeing it now, it has no role whatsoever. Unless I am missing something! Line 2603: channel = MetadataManager.INSTANCE.getChannel(mdTxnCtx, dataverseName, channelName); Appropriate locks must be acquired before accessing metadata entities. I can see why it won't work here but you should acquire the locks here and pass a new flag (lockMetadata = false) when using the created query translator. Otherwise, you risk having some metadata changes happening before you call the translator.compileAndExecute. Then you end up with partial operation which is what we can and must avoid. Line 2624: fieldNames.add("subscriptionId"); make all BAD strings constants and have them in a common place? Line 2641: //Run both statements together to create datasets One of the extremely bad parts in feed which we have agreed that I have to remove is the construction of insert statement this way. Please, find a better way to perform the insert operation. Line 2671: JobSpecification ChanneljobSpec = handleInsertUpsertStatement(metadataProvider, fStatements.get(0), hcc, You're creating a hyracks job that might become invalid when you run it at a later point from a single random NC using count partition constraints of size 1!! Note that the locks obtained for the insert statement is released which means that we are risking doing something really bad on metadata during the insert statement. Why do you need to do it this way? I am sure there are much better ways to do this including simply running the insert query from outside the system. Line 2718: try { what prevents channel datasets from being dropped using a regular drop dataset statement? Line 2726: starting a new transaction before completing the already began transaction? Line 2729: MetadataLockManager.INSTANCE.dropChannelBegin(dataverseName, dataverseName + "." + channelName); why locking twice on the channel? the locking is broken! Line 2745: DropStoredJobOperatorMessage dropChannelMessage = new DropStoredJobOperatorMessage(channelId, channel is dropped after metadata changes. this means that the channel job might still be trying to do failed inserts! Line 2750: } what if the job fails? what do we do then? Line 2803: put commit transaction in a finally clause! Line 2849: use try-catch-finally for metadata transactions Line 2929: boolean isChannel) throws Exception { isChannel is always false! get rid of it? https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/optimizerts/queries/channels/channel-create.aql File asterix-app/src/test/resources/optimizerts/queries/channels/channel-create.aql: Line 2: * Description : Check the Plan used by a channel WSs! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/optimizerts/queries/channels/channel-subscribe.aql File asterix-app/src/test/resources/optimizerts/queries/channels/channel-subscribe.aql: Line 3: * Expected Res : Success WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/optimizerts/queries/channels/channel-unsubscribe.aql File asterix-app/src/test/resources/optimizerts/queries/channels/channel-unsubscribe.aql: Line 5: */ WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/runtimets/queries/channels/create_channel_check_datasets/create_channel_check_datasets.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/channels/create_channel_check_datasets/create_channel_check_datasets.1.ddl.aql: Line 31: return $tweet.message-text WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/runtimets/queries/channels/create_channel_check_datasets/create_channel_check_datasets.3.query.aql File asterix-app/src/test/resources/runtimets/queries/channels/create_channel_check_datasets/create_channel_check_datasets.3.query.aql: Line 4: for $x in dataset Metadata.Dataset WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/runtimets/queries/channels/create_channel_check_metadata/create_channel_check_metadata.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/channels/create_channel_check_metadata/create_channel_check_metadata.1.ddl.aql: Line 31: return $tweet.message-text WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/runtimets/queries/channels/drop_channel_check_datasets/drop_channel_check_datasets.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/channels/drop_channel_check_datasets/drop_channel_check_datasets.1.ddl.aql: Line 31: return $tweet.message-text WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/runtimets/queries/channels/drop_channel_check_datasets/drop_channel_check_datasets.3.query.aql File asterix-app/src/test/resources/runtimets/queries/channels/drop_channel_check_datasets/drop_channel_check_datasets.3.query.aql: Line 4: for $x in dataset Metadata.Dataset WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/runtimets/queries/channels/drop_channel_check_metadata/drop_channel_check_metadata.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/channels/drop_channel_check_metadata/drop_channel_check_metadata.1.ddl.aql: Line 31: return $tweet.message-text WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/runtimets/queries/channels/subscribe_channel_check_subscriptions/subscribe_channel_check_subscriptions.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/channels/subscribe_channel_check_subscriptions/subscribe_channel_check_subscriptions.1.ddl.aql: Line 31: return $tweet.message-text WS! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/runtimets/queries/dml/insert-return-records/insert-return-records.2.update.aql File asterix-app/src/test/resources/runtimets/queries/dml/insert-return-records/insert-return-records.2.update.aql: Line 4: * Expected Result : Success File not needed! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-app/src/test/resources/runtimets/queries/dml/insert-returning-fieldname/insert-returning-fieldname.2.update.aql File asterix-app/src/test/resources/runtimets/queries/dml/insert-returning-fieldname/insert-returning-fieldname.2.update.aql: Line 6: */ File not needed! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-lang-aql/src/main/javacc/AQL.jj File asterix-lang-aql/src/main/javacc/AQL.jj: Line 727: ( this is not good because there has to be exactly a single space between repetitive and channel Line 1179: Pair<Identifier,Identifier> nameComponents = null; WS https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataManager.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataManager.java: Line 61: * WSs! https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Broker.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Broker.java: Line 5: * you may obtain a copy of the License from WS https://asterix-gerrit.ics.uci.edu/#/c/693/7/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Channel.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Channel.java: Line 6: * WS -- To view, visit https://asterix-gerrit.ics.uci.edu/693 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ba31141e9e89e626a3fbe560e08b73e459af16a Gerrit-PatchSet: 7 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Steven Jacobs <[email protected]> Gerrit-Reviewer: Ildar Absalyamov <[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
