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/11/04 03:57:58 UTC
[GitHub] [pinot] jasperjiaguo opened a new pull request, #9727: Server Thread Level Usage Accounting and Query Killing
jasperjiaguo opened a new pull request, #9727:
URL: https://github.com/apache/pinot/pull/9727
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023251290
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
+ private final long _startBytesAllocated;
Review Comment:
The start time/allocatedBytes (reference point) on thread when ThreadResourceUsageProvider is created (task starts on the thread).
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023311988
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountantFactory.java:
##########
@@ -0,0 +1,26 @@
+/**
+ * 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.spi.accounting;
+
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public interface ThreadAccountantFactory {
+ ThreadAccountant init(int numPqr, int numPqw, PinotConfiguration config);
Review Comment:
Now `PerQueryCPUMemResourceUsageAccountant` only relies on total number of threads, as can be used for broker. Keeping interface this way for better extensibility.
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018510317
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
Review Comment:
(nit) suggest adding an empty line before 49 for readability
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018522262
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
+ private final long _startBytesAllocated;
Review Comment:
What all will this cover ?
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023104361
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountantFactory.java:
##########
@@ -0,0 +1,26 @@
+/**
+ * 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.spi.accounting;
+
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public interface ThreadAccountantFactory {
+ ThreadAccountant init(int numPqr, int numPqw, PinotConfiguration config);
Review Comment:
It can still be used on broker by simply setting numPqw to 0.
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023254019
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
Review Comment:
Done
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
Review Comment:
Done
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023259239
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
Review Comment:
Done
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1025788338
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/CPUMemThreadLevelAccountingObjects.java:
##########
@@ -0,0 +1,115 @@
+/**
+ * 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.core.accounting;
+
+import java.util.HashMap;
+import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.accounting.ExecutionContext;
+
+
+/**
+ * Entries for thread level stats and task info collection used on server/broker
+ */
+public class CPUMemThreadLevelAccountingObjects {
+
+ public static class StatsDigest {
+
+ final long[] _statsEntries;
+ final long[] _lastStatSample;
+ final HashMap<String, Long> _finishedTaskStatAggregator;
Review Comment:
added
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023171912
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
+
+ /**
+ * operator call to update thread bytes allocated
+ */
+ void sampleThreadBytesAllocated();
+
+ /**
+ * start the periodical task
+ */
+ void startWatcherTask();
Review Comment:
Right removed this from the interface
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023203104
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
Review Comment:
The hierarchy of `ExecutionContext` is implementation dependent as far as the interface concerns ( the context can point to its anchor thread / be independent per thread / have a pointer to its parent ). In our impl it's per thread. Renamed to `ThreadExecutionContext` for clarity.
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018514254
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
+ private final long _startBytesAllocated;
+
+ public ThreadResourceUsageProvider() {
+ _startTimeNs = _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() : -1;
+
+ long startBytesAllocated1;
+ try {
+ startBytesAllocated1 = _isThreadMemoryMeasurementEnabled
+ ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD.invoke(MX_BEAN, Thread.currentThread().getId()) : -1;
+ } catch (IllegalAccessException | InvocationTargetException e) {
+ startBytesAllocated1 = -1;
+ LOGGER.error("Exception happened during the invocation of getting initial bytes allocated", e);
+ }
+ _startBytesAllocated = startBytesAllocated1;
+ }
+
+ public static boolean isThreadCpuTimeMeasurementEnabled() {
+ return _isThreadCpuTimeMeasurementEnabled;
+ }
+
+ public static void setThreadCpuTimeMeasurementEnabled(boolean enable) {
+ _isThreadCpuTimeMeasurementEnabled = enable && IS_CURRENT_THREAD_CPU_TIME_SUPPORTED;
+ }
+
+ public static boolean isThreadMemoryMeasurementEnabled() {
+ return _isThreadMemoryMeasurementEnabled;
+ }
+
+ public static void setThreadMemoryMeasurementEnabled(boolean enable) {
+ _isThreadMemoryMeasurementEnabled = enable && IS_THREAD_ALLOCATED_MEMORY_SUPPORTED
+ && IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ }
+
+ public long getThreadTimeNs() {
+ return _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() - _startTimeNs : 0;
+ }
+
+ public long getThreadAllocatedBytes() {
+ try {
+ return _isThreadMemoryMeasurementEnabled ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD
+ .invoke(MX_BEAN, Thread.currentThread().getId()) - _startBytesAllocated : 0;
+ } catch (IllegalAccessException | InvocationTargetException e) {
+ LOGGER.error("Exception happened during the invocation of getting initial bytes allocated", e);
+ return 0;
+ }
+ }
+
+ //initialize the com.sun.management.ThreadMXBean related variables using reflection
+ static {
+ Class<?> sunThreadMXBeanClass;
+ try {
+ sunThreadMXBeanClass = Class.forName(SUN_THREAD_MXBEAN_CLASS_NAME);
+ } catch (ClassNotFoundException e) {
+ LOGGER.error("Not able to load com.sun.management.ThreadMXBean, you are probably not using Hotspot jvm");
+ sunThreadMXBeanClass = null;
+ }
+
+ boolean isThreadAllocateMemorySupported = false;
+ try {
+ isThreadAllocateMemorySupported =
+ sunThreadMXBeanClass != null && (boolean) sunThreadMXBeanClass
+ .getMethod(SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME)
+ .invoke(MX_BEAN);
+ } catch (IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
+ LOGGER.error("Not able to call isThreadAllocatedMemorySupported, ", e);
+ }
+ IS_THREAD_ALLOCATED_MEMORY_SUPPORTED = isThreadAllocateMemorySupported;
+
+ boolean isThreadAllocateMemoryEnabled = false;
+ try {
+ isThreadAllocateMemoryEnabled =
+ sunThreadMXBeanClass != null && (boolean) sunThreadMXBeanClass
+ .getMethod(SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME)
+ .invoke(MX_BEAN);
+ } catch (IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
+ LOGGER.error("Not able to call isThreadAllocatedMemoryEnabled, ", e);
+ }
+ IS_THREAD_ALLOCATED_MEMORY_ENABLED = isThreadAllocateMemoryEnabled;
+
+ Method threadAllocateBytes = null;
+ if (IS_THREAD_ALLOCATED_MEMORY_SUPPORTED && IS_THREAD_ALLOCATED_MEMORY_ENABLED) {
+ try {
+ threadAllocateBytes = sunThreadMXBeanClass
+ .getMethod(SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME, long.class);
+ } catch (NoSuchMethodException ignored) {
+ }
+ }
+ SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD = threadAllocateBytes;
+ }
+
+ static {
+ LOGGER.info("Current thread cpu time measurement supported: {}", IS_CURRENT_THREAD_CPU_TIME_SUPPORTED);
+ LOGGER.info("Current thread allocated bytes measurement supported: {}", IS_THREAD_ALLOCATED_MEMORY_SUPPORTED);
+ LOGGER.info("Current thread allocated bytes measurement enabled: {}", IS_THREAD_ALLOCATED_MEMORY_ENABLED);
+ }
Review Comment:
Curious why unlike THREAD_CPU_TIME, THREAD_ALLOCATED_MEMORY needs to have separate info on whether it is enabled and supported ?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
+ private final long _startBytesAllocated;
+
+ public ThreadResourceUsageProvider() {
+ _startTimeNs = _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() : -1;
+
+ long startBytesAllocated1;
+ try {
+ startBytesAllocated1 = _isThreadMemoryMeasurementEnabled
+ ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD.invoke(MX_BEAN, Thread.currentThread().getId()) : -1;
+ } catch (IllegalAccessException | InvocationTargetException e) {
+ startBytesAllocated1 = -1;
+ LOGGER.error("Exception happened during the invocation of getting initial bytes allocated", e);
+ }
+ _startBytesAllocated = startBytesAllocated1;
+ }
+
+ public static boolean isThreadCpuTimeMeasurementEnabled() {
+ return _isThreadCpuTimeMeasurementEnabled;
+ }
+
+ public static void setThreadCpuTimeMeasurementEnabled(boolean enable) {
+ _isThreadCpuTimeMeasurementEnabled = enable && IS_CURRENT_THREAD_CPU_TIME_SUPPORTED;
+ }
+
+ public static boolean isThreadMemoryMeasurementEnabled() {
+ return _isThreadMemoryMeasurementEnabled;
+ }
+
+ public static void setThreadMemoryMeasurementEnabled(boolean enable) {
+ _isThreadMemoryMeasurementEnabled = enable && IS_THREAD_ALLOCATED_MEMORY_SUPPORTED
+ && IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ }
+
+ public long getThreadTimeNs() {
+ return _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() - _startTimeNs : 0;
+ }
+
+ public long getThreadAllocatedBytes() {
+ try {
+ return _isThreadMemoryMeasurementEnabled ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD
+ .invoke(MX_BEAN, Thread.currentThread().getId()) - _startBytesAllocated : 0;
+ } catch (IllegalAccessException | InvocationTargetException e) {
+ LOGGER.error("Exception happened during the invocation of getting initial bytes allocated", e);
+ return 0;
+ }
+ }
+
+ //initialize the com.sun.management.ThreadMXBean related variables using reflection
+ static {
+ Class<?> sunThreadMXBeanClass;
+ try {
+ sunThreadMXBeanClass = Class.forName(SUN_THREAD_MXBEAN_CLASS_NAME);
+ } catch (ClassNotFoundException e) {
+ LOGGER.error("Not able to load com.sun.management.ThreadMXBean, you are probably not using Hotspot jvm");
+ sunThreadMXBeanClass = null;
+ }
+
+ boolean isThreadAllocateMemorySupported = false;
+ try {
+ isThreadAllocateMemorySupported =
+ sunThreadMXBeanClass != null && (boolean) sunThreadMXBeanClass
+ .getMethod(SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME)
+ .invoke(MX_BEAN);
+ } catch (IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
+ LOGGER.error("Not able to call isThreadAllocatedMemorySupported, ", e);
+ }
+ IS_THREAD_ALLOCATED_MEMORY_SUPPORTED = isThreadAllocateMemorySupported;
+
+ boolean isThreadAllocateMemoryEnabled = false;
+ try {
+ isThreadAllocateMemoryEnabled =
+ sunThreadMXBeanClass != null && (boolean) sunThreadMXBeanClass
+ .getMethod(SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME)
+ .invoke(MX_BEAN);
+ } catch (IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
+ LOGGER.error("Not able to call isThreadAllocatedMemoryEnabled, ", e);
+ }
+ IS_THREAD_ALLOCATED_MEMORY_ENABLED = isThreadAllocateMemoryEnabled;
+
+ Method threadAllocateBytes = null;
+ if (IS_THREAD_ALLOCATED_MEMORY_SUPPORTED && IS_THREAD_ALLOCATED_MEMORY_ENABLED) {
+ try {
+ threadAllocateBytes = sunThreadMXBeanClass
+ .getMethod(SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME, long.class);
+ } catch (NoSuchMethodException ignored) {
+ }
+ }
+ SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD = threadAllocateBytes;
+ }
+
+ static {
+ LOGGER.info("Current thread cpu time measurement supported: {}", IS_CURRENT_THREAD_CPU_TIME_SUPPORTED);
+ LOGGER.info("Current thread allocated bytes measurement supported: {}", IS_THREAD_ALLOCATED_MEMORY_SUPPORTED);
+ LOGGER.info("Current thread allocated bytes measurement enabled: {}", IS_THREAD_ALLOCATED_MEMORY_ENABLED);
+ }
Review Comment:
Curious why unlike `THREAD_CPU_TIME`, `THREAD_ALLOCATED_MEMORY` needs to have separate info on whether it is enabled and supported ?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018564803
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
Review Comment:
javadoc please
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018510552
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
Review Comment:
(nit) suggest adding an empty line before 35 for readability
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018606312
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
Review Comment:
Consider abstracting this out further.
Probably accountant does not need to know or be told about what specific things to account. Since accountant relies on ResourceUsageProvider, the latter determines what resource usage will be accounted rather than caller of Accountant telling.
We can provide an abstract collect / account and based on the ResourceUsageProvider plugged into the Accountant, it will be called to sample the resource usage.
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023252164
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
+ private final long _startBytesAllocated;
+
+ public ThreadResourceUsageProvider() {
+ _startTimeNs = _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() : -1;
+
+ long startBytesAllocated1;
+ try {
+ startBytesAllocated1 = _isThreadMemoryMeasurementEnabled
+ ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD.invoke(MX_BEAN, Thread.currentThread().getId()) : -1;
+ } catch (IllegalAccessException | InvocationTargetException e) {
+ startBytesAllocated1 = -1;
+ LOGGER.error("Exception happened during the invocation of getting initial bytes allocated", e);
+ }
+ _startBytesAllocated = startBytesAllocated1;
+ }
+
+ public static boolean isThreadCpuTimeMeasurementEnabled() {
+ return _isThreadCpuTimeMeasurementEnabled;
+ }
+
+ public static void setThreadCpuTimeMeasurementEnabled(boolean enable) {
+ _isThreadCpuTimeMeasurementEnabled = enable && IS_CURRENT_THREAD_CPU_TIME_SUPPORTED;
+ }
+
+ public static boolean isThreadMemoryMeasurementEnabled() {
+ return _isThreadMemoryMeasurementEnabled;
+ }
+
+ public static void setThreadMemoryMeasurementEnabled(boolean enable) {
+ _isThreadMemoryMeasurementEnabled = enable && IS_THREAD_ALLOCATED_MEMORY_SUPPORTED
+ && IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ }
+
+ public long getThreadTimeNs() {
+ return _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() - _startTimeNs : 0;
+ }
+
+ public long getThreadAllocatedBytes() {
+ try {
+ return _isThreadMemoryMeasurementEnabled ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD
+ .invoke(MX_BEAN, Thread.currentThread().getId()) - _startBytesAllocated : 0;
Review Comment:
Yes, see `killMostExpensiveQuery`
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023203104
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
Review Comment:
The hierarchy of `ExecutionContext` is implementation dependent as far as the interface concerns ( the context can point to its anchor thread / be independent per thread / have a pointer to its parent ). In our impl it's per thread. Renamed to `ThreadExecutionContext` for better clarity.
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018568445
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
Review Comment:
Suggest calling it anchor / parent thread since runner-worker terminology is applicable on server and this framework is ideally going to be reusable for broker solution as well.
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018564803
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
Review Comment:
javadoc please
call it `ThreadResourceUsageAccountant` instead ?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018606953
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
+ private final long _startBytesAllocated;
+
+ public ThreadResourceUsageProvider() {
+ _startTimeNs = _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() : -1;
+
+ long startBytesAllocated1;
Review Comment:
(nit) wondering why do we need to first init the local variable here
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023191645
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
Review Comment:
Done
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023203104
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
Review Comment:
The hierarchy of `ExecutionContext` is implementation dependent as far as the interface concerns ( the context can point to its anchor thread / be independent per thread / have a pointer to its parent ). In our impl it's per thread. Renamed to `ThreadExecutionContext`
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018580004
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
+
+ /**
+ * operator call to update thread bytes allocated
+ */
+ void sampleThreadBytesAllocated();
+
+ /**
+ * start the periodical task
+ */
+ void startWatcherTask();
+
+ /**
+ * get error message if the query is killed
+ * @return empty string if N/A
+ */
+ String getErrorMsg();
Review Comment:
Should this be `Exception` or `List<Exception>` ?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018525204
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ExecutionContext.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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.spi.accounting;
+
+public interface ExecutionContext {
Review Comment:
Javadoc please
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1021929832
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
Review Comment:
Good point, will abstract the sampling
--
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 merged pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia merged PR #9727:
URL: https://github.com/apache/pinot/pull/9727
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023312393
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
Review Comment:
Changed all naming of root thread to anchor thread
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9727:
URL: https://github.com/apache/pinot/pull/9727#issuecomment-1302983564
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9727?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 [#9727](https://codecov.io/gh/apache/pinot/pull/9727?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8d62d53) into [master](https://codecov.io/gh/apache/pinot/commit/229c55e65ad7d3e0185c57b5a6da53c8adc0a9d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (229c55e) will **decrease** coverage by `37.73%`.
> The diff coverage is `5.69%`.
```diff
@@ Coverage Diff @@
## master #9727 +/- ##
=============================================
- Coverage 63.02% 25.28% -37.74%
+ Complexity 5229 44 -5185
=============================================
Files 1938 1942 +4
Lines 104130 104606 +476
Branches 15780 15854 +74
=============================================
- Hits 65625 26452 -39173
- Misses 33640 75451 +41811
+ Partials 4865 2703 -2162
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `25.28% <5.69%> (-0.10%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `?` | |
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/9727?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...accounting/CPUMemThreadLevelAccountingObjects.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hY2NvdW50aW5nL0NQVU1lbVRocmVhZExldmVsQWNjb3VudGluZ09iamVjdHMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...counting/HeapUsagePublishingAccountantFactory.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hY2NvdW50aW5nL0hlYXBVc2FnZVB1Ymxpc2hpbmdBY2NvdW50YW50RmFjdG9yeS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...re/accounting/PerQueryCPUMemAccountantFactory.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hY2NvdW50aW5nL1BlclF1ZXJ5Q1BVTWVtQWNjb3VudGFudEZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...unting/utils/RunnerWorkerThreadOffsetProvider.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hY2NvdW50aW5nL3V0aWxzL1J1bm5lcldvcmtlclRocmVhZE9mZnNldFByb3ZpZGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...a/org/apache/pinot/core/operator/BaseOperator.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9CYXNlT3BlcmF0b3IuamF2YQ==) | `80.00% <0.00%> (-20.00%)` | :arrow_down: |
| [...a/org/apache/pinot/core/plan/DocIdSetPlanNode.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0RvY0lkU2V0UGxhbk5vZGUuamF2YQ==) | `80.00% <ø> (ø)` | |
| [...core/query/executor/ServerQueryExecutorV1Impl.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9leGVjdXRvci9TZXJ2ZXJRdWVyeUV4ZWN1dG9yVjFJbXBsLmphdmE=) | `76.02% <0.00%> (-6.95%)` | :arrow_down: |
| [...e/pinot/core/transport/InstanceRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvSW5zdGFuY2VSZXF1ZXN0SGFuZGxlci5qYXZh) | `39.84% <ø> (-24.22%)` | :arrow_down: |
| [...ot/query/runtime/executor/WorkerQueryExecutor.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9leGVjdXRvci9Xb3JrZXJRdWVyeUV4ZWN1dG9yLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/server/conf/ServerConf.java](https://codecov.io/gh/apache/pinot/pull/9727/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvY29uZi9TZXJ2ZXJDb25mLmphdmE=) | `79.31% <ø> (-3.45%)` | :arrow_down: |
| ... and [1223 more](https://codecov.io/gh/apache/pinot/pull/9727/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] siddharthteotia commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018569126
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
+
+ /**
+ * operator call to update thread bytes allocated
+ */
+ void sampleThreadBytesAllocated();
+
+ /**
+ * start the periodical task
+ */
+ void startWatcherTask();
Review Comment:
Wondering if this should be part of the interface. Who will tell the accountant to start watcher task ? As soon as accountant is initialized, I feel it should start the watcher task ?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018509384
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
Review Comment:
(nit) allocate bytes -> `allocatedBytes (JVM heap)`
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023186449
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
+
+ /**
+ * operator call to update thread bytes allocated
+ */
+ void sampleThreadBytesAllocated();
+
+ /**
+ * start the periodical task
+ */
+ void startWatcherTask();
+
+ /**
+ * get error message if the query is killed
+ * @return empty string if N/A
+ */
+ String getErrorMsg();
Review Comment:
This is essentially returning an informative error status issued by the accountant, not exceptions thrown in the code, so probably better to remain as string. I have updated the name to be `getErrorStatus`
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023179370
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
Review Comment:
Good point, updated
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023203104
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
Review Comment:
The hierarchy of `ExecutionContext` is implementation dependent as far as the interface concerns ( the context can point to its anchor thread / be independent per thread / have a pointer to its parent ). In our impl it's per thread.
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023080741
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
+ private final long _startBytesAllocated;
+
+ public ThreadResourceUsageProvider() {
+ _startTimeNs = _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() : -1;
+
+ long startBytesAllocated1;
Review Comment:
because _startBytesAllocated is final so need a intermediate var to do this.
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1024319851
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
+
+ /**
+ * operator call to update thread bytes allocated
+ */
+ void sampleThreadBytesAllocated();
+
+ /**
+ * start the periodical task
+ */
+ void startWatcherTask();
+
+ /**
+ * get error message if the query is killed
+ * @return empty string if N/A
+ */
+ String getErrorMsg();
Review Comment:
Rethought this and changed the return type to exception for better generality and abstraction
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018545020
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ExecutionContext.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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.spi.accounting;
+
+public interface ExecutionContext {
+
+ /**
+ * get query id of the execution context
+ * @return query id in string
+ */
+ String getQueryId();
+
+ /**
+ *
+ * @return get the root thread of execution context
+ */
+ Thread getRootThread();
Review Comment:
Yes in the current context it means the runner thread. In the future if we have a tree structured execution threads it would be the root thread that initiated the whole execution tree for the same query.
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018567752
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
Review Comment:
Can you elaborate a little bit on the ExecutionContext hierarchy ?
taskId is used to submit tasks for segment level (operator tree level) execution to the worker threads. Each worker thread gets one or more segments that they process sequentially.
Is the `taskId` in `createExecutionContext` same as above `taskId` implying per query there are as many ExecutionContext as the number of segments executed by that query ?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018567752
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
Review Comment:
Can you elaborate a little bit on the `ExecutionContext` hierarchy ?
`taskId` is used to submit tasks for segment level (operator tree level) execution to the worker threads. Each worker thread gets one or more segments that they process sequentially.
Is the `taskId` in `createExecutionContext` same as above `taskId` implying per query there are as many ExecutionContext as the number of segments executed by that query ?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018581723
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountantFactory.java:
##########
@@ -0,0 +1,26 @@
+/**
+ * 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.spi.accounting;
+
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public interface ThreadAccountantFactory {
+ ThreadAccountant init(int numPqr, int numPqw, PinotConfiguration config);
Review Comment:
`numPqr -> numRunnerThreads`
`numPqw -> numWorkerThreads`
Having this runner and worker concept in `init` also means that this can't be used on broker ?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018524903
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/CPUMemThreadLevelAccountingObjects.java:
##########
@@ -0,0 +1,115 @@
+/**
+ * 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.core.accounting;
+
+import java.util.HashMap;
+import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.accounting.ExecutionContext;
+
+
+/**
+ * Entries for thread level stats and task info collection used on server/broker
+ */
+public class CPUMemThreadLevelAccountingObjects {
+
+ public static class StatsDigest {
+
+ final long[] _statsEntries;
+ final long[] _lastStatSample;
+ final HashMap<String, Long> _finishedTaskStatAggregator;
Review Comment:
(nit) can you give brief one line comments for future readers of the code who won't know because they may not know the design
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018581723
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountantFactory.java:
##########
@@ -0,0 +1,26 @@
+/**
+ * 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.spi.accounting;
+
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public interface ThreadAccountantFactory {
+ ThreadAccountant init(int numPqr, int numPqw, PinotConfiguration config);
Review Comment:
numPqr -> numRunnerThreads
numPqw -> numWorkerThreads
Having this runner and worker concept in init also means that this can't be used on broker ?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountantFactory.java:
##########
@@ -0,0 +1,26 @@
+/**
+ * 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.spi.accounting;
+
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public interface ThreadAccountantFactory {
+ ThreadAccountant init(int numPqr, int numPqw, PinotConfiguration config);
Review Comment:
numPqr -> numRunnerThreads
numPqw -> numWorkerThreads
Having this runner and worker concept in `init` also means that this can't be used on broker ?
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023171912
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
+
+ /**
+ * operator call to update thread bytes allocated
+ */
+ void sampleThreadBytesAllocated();
+
+ /**
+ * start the periodical task
+ */
+ void startWatcherTask();
Review Comment:
~Right removed this from the interface~
Actually no because we are using something like `Tracing.register(threadAccountantFactory.init(numPqr, numPqw, config));` to register the accountant, as could fail theoretically, in this case the task should not start from `init(...)`
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1023248214
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ExecutionContext.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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.spi.accounting;
+
+public interface ExecutionContext {
Review Comment:
Done
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018509384
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
Review Comment:
(nit) allocate bytes -> allocatedBytes (JVM heap)
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018521765
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
+ private final long _startBytesAllocated;
+
+ public ThreadResourceUsageProvider() {
+ _startTimeNs = _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() : -1;
+
+ long startBytesAllocated1;
+ try {
+ startBytesAllocated1 = _isThreadMemoryMeasurementEnabled
+ ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD.invoke(MX_BEAN, Thread.currentThread().getId()) : -1;
+ } catch (IllegalAccessException | InvocationTargetException e) {
+ startBytesAllocated1 = -1;
+ LOGGER.error("Exception happened during the invocation of getting initial bytes allocated", e);
+ }
+ _startBytesAllocated = startBytesAllocated1;
+ }
+
+ public static boolean isThreadCpuTimeMeasurementEnabled() {
+ return _isThreadCpuTimeMeasurementEnabled;
+ }
+
+ public static void setThreadCpuTimeMeasurementEnabled(boolean enable) {
+ _isThreadCpuTimeMeasurementEnabled = enable && IS_CURRENT_THREAD_CPU_TIME_SUPPORTED;
+ }
+
+ public static boolean isThreadMemoryMeasurementEnabled() {
+ return _isThreadMemoryMeasurementEnabled;
+ }
+
+ public static void setThreadMemoryMeasurementEnabled(boolean enable) {
+ _isThreadMemoryMeasurementEnabled = enable && IS_THREAD_ALLOCATED_MEMORY_SUPPORTED
+ && IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ }
+
+ public long getThreadTimeNs() {
+ return _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() - _startTimeNs : 0;
+ }
+
+ public long getThreadAllocatedBytes() {
+ try {
+ return _isThreadMemoryMeasurementEnabled ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD
+ .invoke(MX_BEAN, Thread.currentThread().getId()) - _startBytesAllocated : 0;
Review Comment:
So if the memory mxbean is not supported, will the caller fallback on cpu time approach ?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018525464
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ExecutionContext.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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.spi.accounting;
+
+public interface ExecutionContext {
+
+ /**
+ * get query id of the execution context
+ * @return query id in string
+ */
+ String getQueryId();
Review Comment:
So ExecutionContext granularity is on a per query basis right ?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ExecutionContext.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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.spi.accounting;
+
+public interface ExecutionContext {
+
+ /**
+ * get query id of the execution context
+ * @return query id in string
+ */
+ String getQueryId();
Review Comment:
So `ExecutionContext` granularity is on a per query basis right ?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018526113
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ExecutionContext.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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.spi.accounting;
+
+public interface ExecutionContext {
+
+ /**
+ * get query id of the execution context
+ * @return query id in string
+ */
+ String getQueryId();
+
+ /**
+ *
+ * @return get the root thread of execution context
+ */
+ Thread getRootThread();
Review Comment:
I am guessing this means runner thread ?
Also, when we want to extend this to broker where we currently don't have a concept of worker thread and runner thread, this rootThread API will continue to work I guess ?
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018544097
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ExecutionContext.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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.spi.accounting;
+
+public interface ExecutionContext {
+
+ /**
+ * get query id of the execution context
+ * @return query id in string
+ */
+ String getQueryId();
Review Comment:
This is per thread basis. Would you think it's better to call this ThreadExecutionContext?
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018581154
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountantFactory.java:
##########
@@ -0,0 +1,26 @@
+/**
+ * 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.spi.accounting;
+
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public interface ThreadAccountantFactory {
+ ThreadAccountant init(int numPqr, int numPqw, PinotConfiguration config);
Review Comment:
(nit) usually Factory provides `create` API
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018606312
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
Review Comment:
Consider abstracting this out further.
Probably accountant does not need to know or be told about what specific things to account. Since accountant relies on `ResourceUsageProvider`, the latter determines what resource usage will be accounted rather than caller of `Accountant` telling.
We can provide an abstract collect / account and based on the `ResourceUsageProvider` plugged into the `Accountant`, it will be called to sample the resource usage.
--
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 #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1018606550
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadAccountant.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.spi.accounting;
+
+import javax.annotation.Nullable;
+
+
+public interface ThreadAccountant {
+
+ /**
+ * clear thread accounting info
+ */
+ void clear();
+
+ /**
+ * check if the corresponding runner thread of current thread is interrupted
+ */
+ boolean isRootThreadInterrupted();
+
+ /**
+ * Task tracking info
+ * @param queryId query id string
+ * @param taskId a unique task id
+ * @param parentContext the parent execution context, null for root(runner) thread
+ */
+ void createExecutionContext(String queryId, int taskId, @Nullable ExecutionContext parentContext);
+
+
+ ExecutionContext getExecutionContext();
+
+ /**
+ * set resource usage provider
+ */
+ void setThreadResourceUsageProvider(ThreadResourceUsageProvider threadResourceUsageProvider);
+
+ /**
+ * operator call to update thread cpu time
+ */
+ void sampleThreadCPUTime();
Review Comment:
The current approach is fine too. Just wondering if this can be made more flexible
--
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] jasperjiaguo commented on a diff in pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on code in PR #9727:
URL: https://github.com/apache/pinot/pull/9727#discussion_r1024618562
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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.spi.accounting;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code ThreadResourceUsageProvider} class providing the functionality of measuring the CPU time
+ * and allocate bytes for the current thread.
+ */
+public class ThreadResourceUsageProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ThreadResourceUsageProvider.class);
+ private static final String SUN_THREAD_MXBEAN_CLASS_NAME = "com.sun.management.ThreadMXBean";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME
+ = "isThreadAllocatedMemorySupported";
+ private static final String SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME
+ = "isThreadAllocatedMemoryEnabled";
+ private static final String SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME = "getThreadAllocatedBytes";
+ private static final Method SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD;
+
+ private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
+ private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_SUPPORTED;
+ private static final boolean IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ private static boolean _isThreadCpuTimeMeasurementEnabled = false;
+ private static boolean _isThreadMemoryMeasurementEnabled = false;
+ private final long _startTimeNs;
+ private final long _startBytesAllocated;
+
+ public ThreadResourceUsageProvider() {
+ _startTimeNs = _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() : -1;
+
+ long startBytesAllocated1;
+ try {
+ startBytesAllocated1 = _isThreadMemoryMeasurementEnabled
+ ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD.invoke(MX_BEAN, Thread.currentThread().getId()) : -1;
+ } catch (IllegalAccessException | InvocationTargetException e) {
+ startBytesAllocated1 = -1;
+ LOGGER.error("Exception happened during the invocation of getting initial bytes allocated", e);
+ }
+ _startBytesAllocated = startBytesAllocated1;
+ }
+
+ public static boolean isThreadCpuTimeMeasurementEnabled() {
+ return _isThreadCpuTimeMeasurementEnabled;
+ }
+
+ public static void setThreadCpuTimeMeasurementEnabled(boolean enable) {
+ _isThreadCpuTimeMeasurementEnabled = enable && IS_CURRENT_THREAD_CPU_TIME_SUPPORTED;
+ }
+
+ public static boolean isThreadMemoryMeasurementEnabled() {
+ return _isThreadMemoryMeasurementEnabled;
+ }
+
+ public static void setThreadMemoryMeasurementEnabled(boolean enable) {
+ _isThreadMemoryMeasurementEnabled = enable && IS_THREAD_ALLOCATED_MEMORY_SUPPORTED
+ && IS_THREAD_ALLOCATED_MEMORY_ENABLED;
+ }
+
+ public long getThreadTimeNs() {
+ return _isThreadCpuTimeMeasurementEnabled ? MX_BEAN.getCurrentThreadCpuTime() - _startTimeNs : 0;
+ }
+
+ public long getThreadAllocatedBytes() {
+ try {
+ return _isThreadMemoryMeasurementEnabled ? (long) SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD
+ .invoke(MX_BEAN, Thread.currentThread().getId()) - _startBytesAllocated : 0;
+ } catch (IllegalAccessException | InvocationTargetException e) {
+ LOGGER.error("Exception happened during the invocation of getting initial bytes allocated", e);
+ return 0;
+ }
+ }
+
+ //initialize the com.sun.management.ThreadMXBean related variables using reflection
+ static {
+ Class<?> sunThreadMXBeanClass;
+ try {
+ sunThreadMXBeanClass = Class.forName(SUN_THREAD_MXBEAN_CLASS_NAME);
+ } catch (ClassNotFoundException e) {
+ LOGGER.error("Not able to load com.sun.management.ThreadMXBean, you are probably not using Hotspot jvm");
+ sunThreadMXBeanClass = null;
+ }
+
+ boolean isThreadAllocateMemorySupported = false;
+ try {
+ isThreadAllocateMemorySupported =
+ sunThreadMXBeanClass != null && (boolean) sunThreadMXBeanClass
+ .getMethod(SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_SUPPORTED_NAME)
+ .invoke(MX_BEAN);
+ } catch (IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
+ LOGGER.error("Not able to call isThreadAllocatedMemorySupported, ", e);
+ }
+ IS_THREAD_ALLOCATED_MEMORY_SUPPORTED = isThreadAllocateMemorySupported;
+
+ boolean isThreadAllocateMemoryEnabled = false;
+ try {
+ isThreadAllocateMemoryEnabled =
+ sunThreadMXBeanClass != null && (boolean) sunThreadMXBeanClass
+ .getMethod(SUN_THREAD_MXBEAN_IS_THREAD_ALLOCATED_MEMORY_ENABLED_NAME)
+ .invoke(MX_BEAN);
+ } catch (IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
+ LOGGER.error("Not able to call isThreadAllocatedMemoryEnabled, ", e);
+ }
+ IS_THREAD_ALLOCATED_MEMORY_ENABLED = isThreadAllocateMemoryEnabled;
+
+ Method threadAllocateBytes = null;
+ if (IS_THREAD_ALLOCATED_MEMORY_SUPPORTED && IS_THREAD_ALLOCATED_MEMORY_ENABLED) {
+ try {
+ threadAllocateBytes = sunThreadMXBeanClass
+ .getMethod(SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_NAME, long.class);
+ } catch (NoSuchMethodException ignored) {
+ }
+ }
+ SUN_THREAD_MXBEAN_GET_BYTES_ALLOCATED_METHOD = threadAllocateBytes;
+ }
+
+ static {
+ LOGGER.info("Current thread cpu time measurement supported: {}", IS_CURRENT_THREAD_CPU_TIME_SUPPORTED);
+ LOGGER.info("Current thread allocated bytes measurement supported: {}", IS_THREAD_ALLOCATED_MEMORY_SUPPORTED);
+ LOGGER.info("Current thread allocated bytes measurement enabled: {}", IS_THREAD_ALLOCATED_MEMORY_ENABLED);
+ }
Review Comment:
Refactored the code on this in the latest commit. `IS_THREAD_ALLOCATED_MEMORY_ENABLED` is a switch that can be turned on during runtime.
--
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 pull request #9727: Thread Level Usage Accounting and Query Killing on Server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #9727:
URL: https://github.com/apache/pinot/pull/9727#issuecomment-1324035346
@jasperjiaguo might want to share some example validation we did internally on PROD use case to demonstrate killing.
Let's also make sure to address any comments that come after the merge.
--
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