Ian Maxon has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/3226 )
Change subject: [ASTERIXDB-2532][RT] per-operator profiling ...................................................................... Patch Set 26: (28 comments) https://asterix-gerrit.ics.uci.edu/#/c/3226/25//COMMIT_MSG Commit Message: https://asterix-gerrit.ics.uci.edu/#/c/3226/25//COMMIT_MSG@9 PS25, Line 9: the analyze : variable is set in a query > I think that it would be more consistent, if we used a parameter for the HT Sure, done. https://asterix-gerrit.ics.uci.edu/#/c/3226/25/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java@85 PS25, Line 85: static fi > Why don't we need to serialize the jobProfile? It actually gets deserialized elsewhere, but I can make it serializable. https://asterix-gerrit.ics.uci.edu/#/c/3226/25/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java@51 PS25, Line 51: profile"), > Wondering if we should just call this "profile". What do you think? Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/aggreg/NestedPlansRunningAggregatorFactory.java File hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/aggreg/NestedPlansRunningAggregatorFactory.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/aggreg/NestedPlansRunningAggregatorFactory.java@69 PS25, Line 69: if (profile) { > It seems that it should be possible to have both wrappers: I tried it this way, and it didn't work. The logic that each wrapper uses to determine whether or not it should wrap needs to be altered for it to work (e.g. the part where you return either fw or TimedFrameWriter.time(fw) depending on instanceof). https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/PipelineAssembler.java File hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/PipelineAssembler.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/PipelineAssembler.java@65 PS25, Line 65: start > it seems that the wrapping code from NestedPlansRunningAggregatorFactory sh Not exactly. Here, I don't want to wrap the micro operators in a timer, I just want to avoid the instanceof overflow if we're profiling and somehow enforce got enabled. It makes the plan rather confusing, for my taste at least, but I can change it if you like. https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/PipelineAssembler.java@108 PS25, Line 108: start > it seems that the wrapping code from NestedPlansRunningAggregatorFactory sh See above. https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IPassableTimer.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IPassableTimer.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IPassableTimer.java@21 PS25, Line 21: IPassableTimer > Add some explanation to this interface? Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedFrameWriter.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedFrameWriter.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedFrameWriter.java@40 PS25, Line 40: this.collector = coll > This should be final, as we synchronize on it. It seems that it should be f Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedFrameWriter.java@43 PS25, Line 43: > Do we actually need the full task context or would the IStatsCollector be s Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedFrameWriter.java@50 PS25, Line 50: > Could we make this constructor call the other constructor? Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedFrameWriter.java@61 PS25, Line 61: final void nextFrame(ByteBuff > I think that it might be better to just throw HyracksDataException up the s Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedFrameWriter.java@56 PS25, Line 56: stopClock(); : } : } : : @Override : > I think that the collector should create new stats if the stats are not tra Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedOperatorNodePushable.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedOperatorNodePushable.java@37 PS25, Line 37: throws > What does this flag indicate? Used to skip the timing in this instance but not used anymore... https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedOperatorNodePushable.java@50 PS25, Line 50: } > same as for TimedFrameWriter Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/TimedOperatorNodePushable.java@129 PS25, Line 129: > Would > 0 work as well? Done, 1 used to mean something different. https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/job/profiling/IStatsCollector.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/job/profiling/IStatsCollector.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/job/profiling/IStatsCollector.java@45 PS25, Line 45: /** > add comment? Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/job/profiling/IStatsCollector.java@53 PS25, Line 53: t every r > add comment? What is the return value? Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/job/profiling/IStatsCollector.java@55 PS25, Line 55: > add comment? Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/job/profiling/StatsCollector.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/job/profiling/StatsCollector.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/job/profiling/StatsCollector.java@42 PS25, Line 42: > If we use an HTTP parameter to request the profile for a request, we don't Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/job/profiling/StatsCollector.java@54 PS25, Line 54: > replace with I just did a different method signature, there are other users of this interface so I assume there must be a reason it is the way it is https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/job/profiling/om/TaskProfile.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/job/profiling/om/TaskProfile.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/job/profiling/om/TaskProfile.java@112 PS25, Line 112: return json; > Could this be a method? Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/job/profiling/om/TaskProfile.java@121 PS25, Line 121: ObjectNode jpe = om.createObje > Does this overwrite the field "counters" that was populated by populateCoun Yeah it does, it is sort of silly I should just override it I suppose. https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/work/StartTasksWork.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/work/StartTasksWork.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/work/StartTasksWork.java@183 PS25, Line 183: (enforce && !profile) > Could we have both? See previous comment https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/group/sort/SortGroupByOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/group/sort/SortGroupByOperatorDescriptor.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/group/sort/SortGroupByOperatorDescriptor.java@153 PS25, Line 153: return > inline? done (i think?) https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/AbstractExternalSortRunMerger.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/AbstractExternalSortRunMerger.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/AbstractExternalSortRunMerger.java@61 PS25, Line 61: public AbstractExternalSortRunMerger(IHyracksTaskContext ctx, ISorter sorter, List<GeneratedRunFileReader> runs, : IBinaryComparator[] comparators, INormalizedKeyComputer nmkComputer, RecordDescriptor recordDesc, : > These seem to be unused. Could we revert the file? Done https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/ExternalSortOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/ExternalSortOperatorDescriptor.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/ExternalSortOperatorDescriptor.java@86 PS25, Line 86: return > inline? done, i think? https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/TopKSorterOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/TopKSorterOperatorDescriptor.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/TopKSorterOperatorDescriptor.java@69 PS25, Line 69: return > inline? done, i think? https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/#/c/3226/25/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java@166 PS25, Line 166: if (this.tupleFilterFactory != null && this.retainMissing) { : throw new IllegalStateException("RetainMissing with tuple filter is not supported"); : } : } : : protected abstract ISearchPredicate createSearchPredicate(); : : protected abstract void resetSearchPredicate(int tupleIndex); : : // Assigns any index-type specific related accessor parameters : > Could be simplified if collector.getOperatorStats would always return an ex Done -- To view, visit https://asterix-gerrit.ics.uci.edu/3226 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie16f3901ae5b32920d8552d5fd1ec8bb6e2ec8ae Gerrit-Change-Number: 3226 Gerrit-PatchSet: 26 Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose (1000171) Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Comment-Date: Tue, 16 Apr 2019 09:43:52 +0000 Gerrit-HasComments: Yes
