>From Ian Maxon <[email protected]>: Attention is currently required from: Ali Alsuliman, Ali Alsuliman.
Ian Maxon has posted comments on this change by Ian Maxon. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20270?usp=email ) Change subject: [ASTERIXDB-3631][RT] Profile nested groupby clauses ...................................................................... Patch Set 8: (2 comments) 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/asterixdb/+/20270/comment/1efdf00f_3d7324e6?usp=email : PS8, Line 38: private final Map<String, IOperatorStats> operatorStatsMap = new ConcurrentHashMap<>(); > "this structure is guarded by the locks around the task, which establish the > correct sequence of eve […] synchronized introduces a lock so that only one thread can enter that section at a time and implies volatile. my view is that we only need the latter. the access pattern of this map is such that as each operator (or micro operator) is instantiated, its stats are added to this collector. afaik this doesn't take place in a concurrent fashion, and happens within one thread. where we run into trouble is that getAllOperatorStats() is called by NotifyTaskCompleteWork in another thread. both of those events are guarded by the synchronization and locks that control the Task lifecycle. you could never complete the task while starting it. so effectively we already have a lock that controls access between writers and readers properly. if we hold the preceding statement to be true, in the best case introducing another lock would simply be redundant. we just need to be sure the JVM doesn't allow the thread running NotifyTaskCompleteWork to read a stale version of this map, i.e. volatile i believe there is already precedent for using a concurrent collection within the TaskContext to achieve this: WarningCollector. that is where i got this idea. i believe what that and StatsCollector are doing is quite similar. File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/group/preclustered/PreclusteredGroupWriter.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20270/comment/02b345da_61cad100?usp=email : PS8, Line 93: aggregatorFactory.setOperatorStats(stats); > My point was we normally don't modify the state of factories and think of > them like immutable things […] yeah, makes sense. i added another signature to create the factory with the stats instead. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20270?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I516dc90b8da3a7086dc80b67946ac14f6ade0973 Gerrit-Change-Number: 20270 Gerrit-PatchSet: 8 Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[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-Attention: Ali Alsuliman <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Comment-Date: Wed, 17 Dec 2025 19:55:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Ian Maxon <[email protected]> Comment-In-Reply-To: Ali Alsuliman <[email protected]>
