You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/12/07 17:55:40 UTC
[GitHub] [pinot] agavra opened a new pull request, #9933: [multistage] add observability logging for thread scheduling
agavra opened a new pull request, #9933:
URL: https://github.com/apache/pinot/pull/9933
improves the observability for operator chain scheduling by adding logs and collecting some additional statistics.
Note:
- `DEBUG` for logs that should only happen once in the lifecycle of an operator chain
- `TRACE` for logs that could happen many times in the lifecycle of an operator chain
Tested this by enabling debug/trace logging and making sure the logs make sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043716120
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
+ */
+public class OpChainStats {
Review Comment:
cc @walterddr about plans to integrate with the multi-stage engine, the reason it's in this PR is because it was moved around during refactoring (first in #9615 and then in this PR) - I'm happy to change it to a simple stopwatch to get time executing on the thread, as with the scheduler design it's pretty trivial to get that information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043684598
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
+ */
+public class OpChainStats {
+
+ // use memoized supplier so that the timing doesn't start until the
+ // first time we get the timer
+ private final Supplier<ThreadResourceUsageProvider> _exTimer
+ = Suppliers.memoize(ThreadResourceUsageProvider::new)::get;
+
+ // this is used to make sure that toString() doesn't have side
+ // effects (accidentally starting the timer)
+ private volatile boolean _exTimerStarted = false;
+
+ private final Stopwatch _queuedStopwatch = Stopwatch.createUnstarted();
+ private final AtomicLong _queuedCount = new AtomicLong();
+
+ private final String _id;
Review Comment:
requestId + stageId
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043687070
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -117,12 +123,26 @@ public void runJob() {
*/
public final void register(OpChain operatorChain) {
register(operatorChain, true);
+ LOGGER.debug("({}): Scheduler is now handling operator chain listening to mailboxes {}. "
Review Comment:
Can this be info level or will that be too noisy ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043681658
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
+ */
+public class OpChainStats {
+
+ // use memoized supplier so that the timing doesn't start until the
+ // first time we get the timer
+ private final Supplier<ThreadResourceUsageProvider> _exTimer
+ = Suppliers.memoize(ThreadResourceUsageProvider::new)::get;
+
+ // this is used to make sure that toString() doesn't have side
+ // effects (accidentally starting the timer)
+ private volatile boolean _exTimerStarted = false;
+
+ private final Stopwatch _queuedStopwatch = Stopwatch.createUnstarted();
+ private final AtomicLong _queuedCount = new AtomicLong();
+
+ private final String _id;
Review Comment:
Is this query `requestId` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1042517509
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -78,29 +78,35 @@ protected void run()
}
OpChain operatorChain = _scheduler.next();
+ LOGGER.trace("({}): Scheduling", operatorChain);
_workerPool.submit(new TraceRunnable() {
@Override
public void runJob() {
try {
- ThreadResourceUsageProvider timer = operatorChain.getAndStartTimer();
+ LOGGER.trace("({}): Executing", operatorChain);
+ operatorChain.getStats().executing();
// so long as there's work to be done, keep getting the next block
// when the operator chain returns a NOOP block, then yield the execution
// of this to another worker
TransferableBlock result = operatorChain.getRoot().nextBlock();
while (!result.isNoOpBlock() && !result.isEndOfStreamBlock()) {
- LOGGER.debug("Got block with {} rows.", result.getNumRows());
Review Comment:
I removed this because it always returns `0` - `MailboxSendOperator` either returns `No-op` or `EOS`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr merged pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
walterddr merged PR #9933:
URL: https://github.com/apache/pinot/pull/9933
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1042739479
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/RoundRobinScheduler.java:
##########
@@ -95,7 +97,14 @@ public boolean hasNext() {
@Override
public OpChain next() {
- return _ready.poll();
+ OpChain op = _ready.poll();
+ trace("Polled " + op);
+ return op;
+ }
+
+ @Override
+ public int size() {
+ return _ready.size() + _available.size();
Review Comment:
Can we report ready q and available q size separately?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1042784397
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/RoundRobinScheduler.java:
##########
@@ -95,7 +97,14 @@ public boolean hasNext() {
@Override
public OpChain next() {
- return _ready.poll();
+ OpChain op = _ready.poll();
+ trace("Polled " + op);
+ return op;
+ }
+
+ @Override
+ public int size() {
+ return _ready.size() + _available.size();
Review Comment:
I do - see the other log lines. This is just for getting the total size (used in a different log message)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1042784535
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/RoundRobinScheduler.java:
##########
@@ -95,7 +97,14 @@ public boolean hasNext() {
@Override
public OpChain next() {
- return _ready.poll();
+ OpChain op = _ready.poll();
+ trace("Polled " + op);
+ return op;
+ }
+
+ @Override
+ public int size() {
+ return _ready.size() + _available.size();
Review Comment:
it also can't be part of the interface because it's implementation specific 😢
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/RoundRobinScheduler.java:
##########
@@ -64,6 +64,7 @@ public void register(OpChain operatorChain, boolean isNew) {
// immediately be considered ready in case it does not need
// read from any mailbox (e.g. with a LiteralValueOperator)
(isNew ? _ready : _available).add(operatorChain);
+ trace("registered " + operatorChain);
Review Comment:
yup
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1042701595
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainScheduler.java:
##########
@@ -55,4 +55,9 @@ public interface OpChainScheduler {
* prior to this call
*/
OpChain next();
+
+ /**
+ * @return the number of operator chains that are awaiting execution
+ */
+ int size();
Review Comment:
there's no need for lock because OCSchedulers are not thread safe - thread safety should come from caller if they care about it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043683720
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
Review Comment:
To clarify, the granularity of this is at the `Stage` level right ?
I mean a particular instance of OpChainStats will track the stats of operator chain executing for a particular Stage on a given host ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043694643
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
+ */
+public class OpChainStats {
Review Comment:
So, if we plan on using the accountant and resource usage (cpu time and memory) provider in the multi stage engine, I don't think this wiring is enough.
Based on some recent discussion, I am guessing there is plan / desire to integrate it into multi stage engine ?
We will have to setup anchor points and collect stats from all the worker threads executing for intermediary layer and above. Leaf layer is already taken care of.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043686380
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
+ */
+public class OpChainStats {
Review Comment:
I think it is fine to use this provider here.
Curious where exactly are you going to call `getThreadTimeNs` (accounted cpu time) and `getThreadAllocatedBytes` (accounted memory) because that is what it is being used for by the accountant in the current engine
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043692879
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
+ */
+public class OpChainStats {
Review Comment:
for now it's only in the `toString` method, but later we can use it when reporting metrics
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9933:
URL: https://github.com/apache/pinot/pull/9933#issuecomment-1341403902
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9933?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9933](https://codecov.io/gh/apache/pinot/pull/9933?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (07f2245) into [master](https://codecov.io/gh/apache/pinot/commit/e6a9881d7d1b397934983756ca4f14f3419d6824?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e6a9881) will **decrease** coverage by `53.08%`.
> The diff coverage is `86.79%`.
```diff
@@ Coverage Diff @@
## master #9933 +/- ##
=============================================
- Coverage 68.97% 15.88% -53.09%
+ Complexity 5058 176 -4882
=============================================
Files 1982 1929 -53
Lines 106433 104082 -2351
Branches 16127 15849 -278
=============================================
- Hits 73409 16531 -56878
- Misses 27877 86335 +58458
+ Partials 5147 1216 -3931
```
| Flag | Coverage Δ | |
|---|---|---|
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `15.88% <86.79%> (+0.05%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9933?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...che/pinot/query/runtime/operator/OpChainStats.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9PcENoYWluU3RhdHMuamF2YQ==) | `78.26% <78.26%> (ø)` | |
| [...uery/runtime/executor/OpChainSchedulerService.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9leGVjdXRvci9PcENoYWluU2NoZWR1bGVyU2VydmljZS5qYXZh) | `90.90% <87.50%> (+2.53%)` | :arrow_up: |
| [...ot/query/runtime/executor/RoundRobinScheduler.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9leGVjdXRvci9Sb3VuZFJvYmluU2NoZWR1bGVyLmphdmE=) | `100.00% <100.00%> (ø)` | |
| [...g/apache/pinot/query/runtime/operator/OpChain.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9PcENoYWluLmphdmE=) | `100.00% <100.00%> (ø)` | |
| [.../pinot/query/runtime/plan/PhysicalPlanVisitor.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL1BoeXNpY2FsUGxhblZpc2l0b3IuamF2YQ==) | `97.14% <100.00%> (ø)` | |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...n/java/org/apache/pinot/core/data/table/Table.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1462 more](https://codecov.io/gh/apache/pinot/pull/9933/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043684902
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
Review Comment:
yes, that's correct 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043691963
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -117,12 +123,26 @@ public void runJob() {
*/
public final void register(OpChain operatorChain) {
register(operatorChain, true);
+ LOGGER.debug("({}): Scheduler is now handling operator chain listening to mailboxes {}. "
Review Comment:
I think it's likely to be too noisy - `it's O(Num_queries * num_stages_per_query)`. I separated it from the trace calls though so that if you wanted to you could turn on only the once-per-opchain logs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1042544937
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
+ */
+public class OpChainStats {
Review Comment:
i am not familiar with this and how the ThreadResourceUsageProvider is being used. maybe @siddharthteotia @jasperjiaguo can comment on this. thanks
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainScheduler.java:
##########
@@ -55,4 +55,9 @@ public interface OpChainScheduler {
* prior to this call
*/
OpChain next();
+
+ /**
+ * @return the number of operator chains that are awaiting execution
+ */
+ int size();
Review Comment:
this is a weak reference to the queue yes? e.g. no need for lock
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1042738870
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/RoundRobinScheduler.java:
##########
@@ -64,6 +64,7 @@ public void register(OpChain operatorChain, boolean isNew) {
// immediately be considered ready in case it does not need
// read from any mailbox (e.g. with a LiteralValueOperator)
(isNew ? _ready : _available).add(operatorChain);
+ trace("registered " + operatorChain);
Review Comment:
This trace info will be reported as part of the LOGGER.trace?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9933: [multistage] add observability logging for thread scheduling
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9933:
URL: https://github.com/apache/pinot/pull/9933#discussion_r1043696556
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChainStats.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.query.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Suppliers;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.pinot.spi.accounting.ThreadResourceUsageProvider;
+
+
+/**
+ * {@code OpChainStats} tracks execution statistics for {@link OpChain}s.
+ */
+public class OpChainStats {
Review Comment:
> for now it's only in the toString method, but later we can use it when reporting metrics
Got it. My comment crossed with yours. I think we are on the same page that full integration of accounting has to be done separately
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org