You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2020/02/13 15:47:36 UTC
[GitHub] [hive] szlta opened a new pull request #909: HIVE-22821
szlta opened a new pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r381441375
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -4215,6 +4215,12 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
LLAP_IO_CVB_BUFFERED_SIZE("hive.llap.io.cvb.memory.consumption.", 1L << 30,
"The amount of bytes used to buffer CVB between IO and Processor Threads default to 1GB, "
+ "this will be used to compute a best effort queue size for VRBs produced by a LLAP IO thread."),
+ LLAP_IO_PROACTIVE_EVICTION_ENABLED("hive.llap.io.proactive.eviction.enabled", true,
Review comment:
i do not think this is needed we have lot of configs, let it on by default, unless you see a reason to turn it off from operation perspective ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389021342
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -4215,6 +4215,12 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
LLAP_IO_CVB_BUFFERED_SIZE("hive.llap.io.cvb.memory.consumption.", 1L << 30,
"The amount of bytes used to buffer CVB between IO and Processor Threads default to 1GB, "
+ "this will be used to compute a best effort queue size for VRBs produced by a LLAP IO thread."),
+ LLAP_IO_PROACTIVE_EVICTION_ENABLED("hive.llap.io.proactive.eviction.enabled", true,
+ "If true proactive cache eviction is enabled, thus LLAP will proactively evict buffers" +
+ " that belong to dropped Hive entities (DBs, tables, partitions, or temp tables."),
+ LLAP_IO_PROACTIVE_EVICTION_ASYNC("hive.llap.io.proactive.eviction.async", true,
Review comment:
please see comment above, the RPC pool is meant to do that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r396825148
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +226,7 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+
Review comment:
not needed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389559046
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
Well I agree on the daemon side, but not on iHS2. I think this should not handled the same way purge is:
- the invocation of purge is done by the users themselves so they should know what they're doing and this can wait for the result however long it takes
- proactive eviction is done in the background i.e. less apparent to the users - e.g. whenever a drop table/partition/db happens. I'd imagine a use case where we have many many daemons with large caches, and such proactive eviction could take longer. In this case I don't want users having to wait for the eviction to complete before their drop table command returns.
Wouldn't you agree?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r401747089
##########
File path: service/src/java/org/apache/hive/service/server/HiveServer2.java
##########
@@ -61,6 +61,7 @@
import org.apache.hadoop.hive.common.ZooKeeperHiveHelper;
import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.apache.hadoop.hive.llap.ProactiveEviction;
Review comment:
is this needed ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r381452847
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
##########
@@ -333,6 +333,23 @@ public GetTokenResponseProto getDelegationToken(RpcController controller,
}
}
+ @Override
+ public LlapDaemonProtocolProtos.EvictEntityResponseProto evictEntity(
+ RpcController controller, LlapDaemonProtocolProtos.EvictEntityRequestProto protoRequest)
+ throws ServiceException {
+ LlapDaemonProtocolProtos.EvictEntityResponseProto.Builder responseProtoBuilder =
+ LlapDaemonProtocolProtos.EvictEntityResponseProto.newBuilder();
+
+ LlapIo<?> llapIo = LlapProxy.getIo();
+ if (llapIo != null) {
+ llapIo.evictEntity(protoRequest);
+ responseProtoBuilder.setAck(true);
Review comment:
how about we do like for purge we return the amount of data purged ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r388200796
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
##########
@@ -333,6 +333,23 @@ public GetTokenResponseProto getDelegationToken(RpcController controller,
}
}
+ @Override
+ public LlapDaemonProtocolProtos.EvictEntityResponseProto evictEntity(
+ RpcController controller, LlapDaemonProtocolProtos.EvictEntityRequestProto protoRequest)
+ throws ServiceException {
+ LlapDaemonProtocolProtos.EvictEntityResponseProto.Builder responseProtoBuilder =
+ LlapDaemonProtocolProtos.EvictEntityResponseProto.newBuilder();
+
+ LlapIo<?> llapIo = LlapProxy.getIo();
+ if (llapIo != null) {
+ llapIo.evictEntity(protoRequest);
+ responseProtoBuilder.setAck(true);
Review comment:
Yes, ff we opt to get rid of async stuff this will make sense to do
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r388199710
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -4215,6 +4215,12 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
LLAP_IO_CVB_BUFFERED_SIZE("hive.llap.io.cvb.memory.consumption.", 1L << 30,
"The amount of bytes used to buffer CVB between IO and Processor Threads default to 1GB, "
+ "this will be used to compute a best effort queue size for VRBs produced by a LLAP IO thread."),
+ LLAP_IO_PROACTIVE_EVICTION_ENABLED("hive.llap.io.proactive.eviction.enabled", true,
+ "If true proactive cache eviction is enabled, thus LLAP will proactively evict buffers" +
+ " that belong to dropped Hive entities (DBs, tables, partitions, or temp tables."),
+ LLAP_IO_PROACTIVE_EVICTION_ASYNC("hive.llap.io.proactive.eviction.async", true,
Review comment:
As described above: wanted to offload the RPC pool as well. I'm not sure either that it adds value so we can remove it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r406044475
##########
File path: llap-common/src/protobuf/LlapDaemonProtocol.proto
##########
@@ -231,11 +231,15 @@ message SetCapacityRequestProto {
message SetCapacityResponseProto {
}
+// Used for proactive eviction request. Must contain one DB name, and optionally table information.
message EvictEntityRequestProto {
required string db_name = 1;
repeated TableProto table = 2;
}
+// Used in EvictEntityRequestProto, can be used for non-partitioned and partitioned tables too.
+// For the latter part_key contains only the keys, part_val has the values for all partitions on all keys:
+// i.e.: part_key: [pk0, pk1, pk2], part_val: [p00, p01, p02, p10, p11, p12]
Review comment:
`// For partitions pk0=p00/pk1=p01/pk2=p02 and pk0=p10/pk1=p11/pk2=p12`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r396744224
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
You can check this link about[ Double checking locking](https://wiki.sei.cmu.edu/confluence/display/java/LCK10-J.+Use+a+correct+form+of+the+double-checked+locking+idiom)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r396568344
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
Thanks for the detailed comments! I agree with all the points above, I did miss the fact that I was creating a new pool at each invocation - my bad.
I've amended the change with https://github.com/szlta/hive/commit/74364b022f14edfa597a7d95b0774def8f3ddefd to fix this and address the other points 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389987094
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
Again this is a premature Optimization, evicting from the cache is walking a list of couple of millions items at most, that is a fraction of second. Then if this is a real issue it would be better to deal with it within HS2 by making this fire and forget kind of request.
Again i do not see why this is needed and why it is done on the llap side.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r381442289
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -4215,6 +4215,12 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
LLAP_IO_CVB_BUFFERED_SIZE("hive.llap.io.cvb.memory.consumption.", 1L << 30,
"The amount of bytes used to buffer CVB between IO and Processor Threads default to 1GB, "
+ "this will be used to compute a best effort queue size for VRBs produced by a LLAP IO thread."),
+ LLAP_IO_PROACTIVE_EVICTION_ENABLED("hive.llap.io.proactive.eviction.enabled", true,
+ "If true proactive cache eviction is enabled, thus LLAP will proactively evict buffers" +
+ " that belong to dropped Hive entities (DBs, tables, partitions, or temp tables."),
+ LLAP_IO_PROACTIVE_EVICTION_ASYNC("hive.llap.io.proactive.eviction.async", true,
Review comment:
Am not sure am getting this, it is async in what sense ? the request is handled by the rpc framework that is running stuff asynch, i think this is not needed, or please help me understand which thread you are thinking about avoiding ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r394089108
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
To be more clear about point one you can read the [java spec](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ExecutorService.html) it is always very good point to start **An unused ExecutorService should be shut down to allow reclamation of its resources.**
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r394089629
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
I hope you are convinced that Asynchronous stuff can be trick to get right and most of the benefit can turn into debugging nightmare. Let me know if you have more questions
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389958291
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
Okay, so suppose you use beeline and do a drop table statement:
A thread from HiveServer2-Handler-Pool will pick up the incoming Thrift request and execute.
Execution will go into DropTableOperation. Anything happening here will prevent this thread from finishing and thus you as the user on the beeline side (as thrift connects you) will have to wait.
If we invoke a proactive eviction from DropTableOperation (which this change does) obviously the user on beeline side will see this addition processing time for their query.
Now if this additional time is long - that could be bad, since they only wanted to drop a table and now they're waiting extra seconds if the DropTableOperation 'waits' for many LLAP daemons to finish their eviction on their big cache structure.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r401741332
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelLrfuCachePolicy.java
##########
@@ -240,6 +241,12 @@ public void setEvictionListener(EvictionListener listener) {
this.evictionListener = listener;
}
+ @Override
+ public long evictEntity(Predicate<LlapCacheableBuffer> predicate) {
+ // TODO
Review comment:
Same here please add the link to the todo jira.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389020955
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
@szlta , if for every new feature we add a new thread pool then the code will become a huge burden to maintain. The RPC thread pool is meant for that doing the stuff within thread of the pool that can be increased if needed. By creating a new pool we are not changing anything just adding more burden from code perspective. Same on the HS2I user will need to wait and see the result of the command same with the purge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r394083474
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
This code has multiple issues and it is not safe.
1. First issue, there is no explicit shutdown of the executor and that is a big issue by it self, because some implementations shutdown themselves in a finalizer **BUT JVM spec do not guaranteed to ever be call finalize()**.
2. Second this code is creating a thread pool on each call, that is a huge resource wast.
3. Third point Threads are not named thus makes it hard to debug.
4. Fourth assume some of those tasks hangs on IO what will happen ? will the JVM ever stops ?
5. Nth issue Assume assume we have 500 llap nodes (yes we have cluster with 500 node running), this will fire 500 threads is that okay ?
IMO First cut of this pr should avoid all this Async stuff and start with a simple version OR you need to have a static fixed thread pool with a well defined **life cycle** and clear strategy on how to deal with **blocking IO**, **errors** and **shutdown**
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r381452125
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelFifoCachePolicy.java
##########
@@ -71,6 +72,11 @@ public long purge() {
return evicted;
}
+ @Override
+ public long evictEntity(Predicate<LlapCacheableBuffer> predicate) {
+ return 0;
Review comment:
this is missing thought ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r381440128
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
@szlta i do not think we need this, the actual eviction will run on the RPC thread anyway so that is not touching the execution.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r396792555
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
Makes sense. I guess we can do something like this then: https://github.com/apache/hive/pull/909/commits/4b285ef48e95f0a7b83029ab42de7a535d93d4d5
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r381451935
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCachePolicy.java
##########
@@ -75,4 +78,6 @@
* @return amount (bytes) of memory evicted.
*/
long purge();
+
Review comment:
can we add some docs about how the predicate is used, is it evicting when true or false ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389787317
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
Like I said, I'm fine with relying on RPC threadpool only on the LLAP daemon side. I'm not talking about that anymore (you can see I amended that change already)
What I'm talking about now is HS2 side:
- purge currently will block user side: https://github.com/szlta/hive/blob/HIVE-22821/ql/src/java/org/apache/hadoop/hive/ql/processors/LlapCacheResourceProcessor.java#L141
- while proactive eviction - I'd propose - should not: https://github.com/szlta/hive/blob/HIVE-22821/ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java#L79
That's the difference I'm interested in.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389992869
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
The other element that this will entail as well is that will have 2 paths one for the case the user has issue drop table form the cache and the second when the table is dropped via command. IMO when to tackle the first one, where the user is actually issuing a command to explicitly drop the entries form the cache. The second one is a bonus..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r388199821
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCachePolicy.java
##########
@@ -75,4 +78,6 @@
* @return amount (bytes) of memory evicted.
*/
long purge();
+
Review comment:
Will do
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r388199243
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -4215,6 +4215,12 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
LLAP_IO_CVB_BUFFERED_SIZE("hive.llap.io.cvb.memory.consumption.", 1L << 30,
"The amount of bytes used to buffer CVB between IO and Processor Threads default to 1GB, "
+ "this will be used to compute a best effort queue size for VRBs produced by a LLAP IO thread."),
+ LLAP_IO_PROACTIVE_EVICTION_ENABLED("hive.llap.io.proactive.eviction.enabled", true,
Review comment:
I'd keep this - should anything go wrong we'll have an option to turn this feature off, it's better to be prepared for the strangest customer use cases too where this proactive eviction might not make any perf gain
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389995214
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
"better to deal with it within HS2 by making this fire and forget kind of request"
This is exactly my intention and how the code is right now, as you can see this if you read back and take a look on the code.
"fraction of second"
For one daemon, yes - what if we have many? And how will the remote invocations' overhead add up to this?
"why it is done on the llap side"
I already stated 2 times that I'm fine and agree with you on not doing anything additional async stuff on LLAP daemon side, btw did you not see this commit in this PR?
https://github.com/apache/hive/pull/909/commits/ebf39c472dd311e9b7b3d93061736fcac3e20240
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r388197920
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
I guess I just wanted to make sure that this operation is completely decoupled from the rest of threads.
On daemon side you're right we'd be in an RPC handler thread while doing this eviction. My idea was that this request should return as fast as possible and don't even interfere with the RPC threads. Hence the void return value btw.
On HS2 side in ProactiveEviction we already use an executor service to send the eviction request.
So it's decoupled on HS2 too. If you feel that's enough I'll remove the additional async part on the daemon side.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r396742513
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
@szlta the new commit introduced new problem, The problem is that this does not work when using multiple threads each thread will see a null executor and each one will create a new one and same with the shutdown method.
One way to fix this is to make the executor static and final, therefore will be constructed at the time class loading. Also shutdown hook might be needed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r388200533
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelFifoCachePolicy.java
##########
@@ -71,6 +72,11 @@ public long purge() {
return evicted;
}
+ @Override
+ public long evictEntity(Predicate<LlapCacheableBuffer> predicate) {
+ return 0;
Review comment:
That will be the scope of the next jira - HIVE-22821 is only meant to make place of this feature endpoint-wise.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r401751572
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,327 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+import org.apache.hive.common.util.ShutdownHookManager;
+
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ static {
+ ShutdownHookManager.addShutdownHook(new Runnable() {
+ @Override
+ public void run() {
+ if (EXECUTOR != null) {
+ EXECUTOR.shutdownNow();
+ }
+ }
+ });
+ }
+
+ private static final ExecutorService EXECUTOR = Executors.newSingleThreadExecutor(
+ new ThreadFactoryBuilder().setNameFormat("Proactive-Eviction-Requester").setDaemon(true).build());
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ for (LlapServiceInstance instance : instances) {
+ Task task = new Task(conf, instance, request);
+ EXECUTOR.execute(task);
+ }
+
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * The executable task to carry out request sending.
+ */
+ public static class Task implements Runnable {
Review comment:
There is at least 2 classed named Task in the Hive exec module can you rename this to something more specific
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r396744224
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
You can check this link about Double checking locking https://en.wikipedia.org/wiki/Double-checked_locking
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r401741044
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelFifoCachePolicy.java
##########
@@ -71,6 +72,11 @@ public long purge() {
return evicted;
}
+ @Override
+ public long evictEntity(Predicate<LlapCacheableBuffer> predicate) {
+ return 0;
Review comment:
can you please file the jira and add the todo link to make this formal that this is a WIP.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r401479045
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
Hi @b-slim can you take a look on the recent commit 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r394089275
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ ExecutorService executorService = Executors.newCachedThreadPool();
Review comment:
For number 4 you can refer to this spec https://wiki.sei.cmu.edu/confluence/display/java/TPS02-J.+Ensure+that+tasks+submitted+to+a+thread+pool+are+interruptible
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r396825072
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -23,12 +23,16 @@
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
Review comment:
this is unused
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389826419
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
I do think you see my point, by adding this new pool what are you actually unlocking/unclocking ?
Which Threads/actors are blocked and how this is helping?
When you say HS2, can you explain what is blocked there and how that have an impact on the system ?
What am trying to say is that there is nothing to really unblock and this is adding some code that is not needed IMO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r401746158
##########
File path: ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java
##########
@@ -0,0 +1,327 @@
+/*
+ * 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.hadoop.hive.llap;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import javax.net.SocketFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.io.CacheTag;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.daemon.rpc.LlapDaemonProtocolProtos;
+import org.apache.hadoop.hive.llap.impl.LlapManagementProtocolClientImpl;
+import org.apache.hadoop.hive.llap.registry.LlapServiceInstance;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.io.retry.RetryPolicies;
+import org.apache.hadoop.io.retry.RetryPolicy;
+import org.apache.hadoop.net.NetUtils;
+import org.apache.hive.common.util.ShutdownHookManager;
+
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Through this class the caller (typically HS2) can request eviction of buffers from LLAP cache by specifying a DB,
+ * table or partition name/(value). Request sending is implemented here.
+ */
+public final class ProactiveEviction {
+
+ static {
+ ShutdownHookManager.addShutdownHook(new Runnable() {
+ @Override
+ public void run() {
+ if (EXECUTOR != null) {
+ EXECUTOR.shutdownNow();
+ }
+ }
+ });
+ }
+
+ private static final ExecutorService EXECUTOR = Executors.newSingleThreadExecutor(
+ new ThreadFactoryBuilder().setNameFormat("Proactive-Eviction-Requester").setDaemon(true).build());
+
+ private ProactiveEviction() {
+ // Not to be used;
+ }
+
+ /**
+ * Trigger LLAP cache eviction of buffers related to entities residing in request parameter.
+ * @param conf
+ * @param request
+ */
+ public static void evict(Configuration conf, Request request) {
+ if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.LLAP_IO_PROACTIVE_EVICTION_ENABLED)) {
+ return;
+ }
+
+ try {
+ LlapRegistryService llapRegistryService = LlapRegistryService.getClient(conf);
+ Collection<LlapServiceInstance> instances = llapRegistryService.getInstances().getAll();
+ if (instances.size() == 0) {
+ // Not in LLAP mode.
+ return;
+ }
+ for (LlapServiceInstance instance : instances) {
+ Task task = new Task(conf, instance, request);
+ EXECUTOR.execute(task);
+ }
+
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * The executable task to carry out request sending.
+ */
+ public static class Task implements Runnable {
+ private static final Logger LOG = LoggerFactory.getLogger(Task.class);
+ private final Request request;
+ private Configuration conf;
+ private LlapServiceInstance instance;
+ private SocketFactory socketFactory;
+ private RetryPolicy retryPolicy;
+
+ Task(Configuration conf, LlapServiceInstance llapServiceInstance, Request request) {
+ this.conf = conf;
+ this.instance = llapServiceInstance;
+ this.socketFactory = NetUtils.getDefaultSocketFactory(conf);
+ //not making this configurable, best effort
+ this.retryPolicy = RetryPolicies.retryUpToMaximumTimeWithFixedSleep(
+ 10000, 2000L, TimeUnit.MILLISECONDS);
+ this.request = request;
+ }
+
+ @Override
+ public void run() {
+ if (request.isEmpty()) {
+ throw new IllegalArgumentException("No entities set to trigger eviction on.");
+ }
+ try {
+ LlapManagementProtocolClientImpl client = new LlapManagementProtocolClientImpl(conf, instance.getHost(),
+ instance.getManagementPort(), retryPolicy, socketFactory);
+
+ List<LlapDaemonProtocolProtos.EvictEntityRequestProto> protoRequests = request.toProtoRequests();
+
+ for (LlapDaemonProtocolProtos.EvictEntityRequestProto protoRequest : protoRequests) {
+ client.evictEntity(null, protoRequest);
+ }
+ LOG.info("Requested proactive eviction.");
+ } catch (Exception e) {
+ LOG.warn("Exception while requesting proactive eviction.", e);
+ }
+ }
+ }
+
+ /**
+ * Holds information on entities: DB name(s), table name(s), partitions.
+ */
+ public static final class Request {
+
+ private final Map<String, Map<String, Set<LinkedHashMap<String, String>>>> entities;
+
+ private Request(Map<String, Map<String, Set<LinkedHashMap<String, String>>>> entities) {
+ this.entities = entities;
+ }
+
+ public Map<String, Map<String, Set<LinkedHashMap<String, String>>>> getEntities() {
+ return entities;
+ }
+
+ public boolean isEmpty() {
+ return entities.isEmpty();
+ }
+
+ /**
+ * Request often times only contains tables/partitions of 1 DB only.
+ * @return the single DB name, null if the count of DBs present is not exactly 1.
+ */
+ public String getSingleDbName() {
+ if (entities.size() == 1) {
+ return entities.keySet().stream().findFirst().get();
+ }
+ return null;
+ }
+
+ /**
+ * Translate to Protobuf requests.
+ * @return list of request instances ready to be sent over protobuf.
+ */
+ public List<LlapDaemonProtocolProtos.EvictEntityRequestProto> toProtoRequests() {
+
+ List<LlapDaemonProtocolProtos.EvictEntityRequestProto> protoRequests = new LinkedList<>();
+
+ for (Map.Entry<String, Map<String, Set<LinkedHashMap<String, String>>>> dbEntry : entities.entrySet()) {
+ String dbName = dbEntry.getKey();
+ Map<String, Set<LinkedHashMap<String, String>>> tables = dbEntry.getValue();
+
+ LlapDaemonProtocolProtos.EvictEntityRequestProto.Builder requestBuilder =
+ LlapDaemonProtocolProtos.EvictEntityRequestProto.newBuilder();
+ LlapDaemonProtocolProtos.TableProto.Builder tableBuilder = null;
+
+ requestBuilder.setDbName(dbName.toLowerCase());
+ for (Map.Entry<String, Set<LinkedHashMap<String, String>>> tableEntry : tables.entrySet()) {
+ String tableName = tableEntry.getKey();
+ tableBuilder = LlapDaemonProtocolProtos.TableProto.newBuilder();
+ tableBuilder.setTableName(tableName.toLowerCase());
+
+ Set<LinkedHashMap<String, String>> partitions = tableEntry.getValue();
+ Set<String> partitionKeys = null;
+
+ for (Map<String, String> partitionSpec : partitions) {
+ if (partitionKeys == null) {
+ // For a given table the set of partition columns (keys) should not change.
+ partitionKeys = new LinkedHashSet<>(partitionSpec.keySet());
+ tableBuilder.addAllPartKey(partitionKeys);
+ }
+ for (String partKey : tableBuilder.getPartKeyList()) {
+ tableBuilder.addPartVal(partitionSpec.get(partKey));
+ }
+ }
+ requestBuilder.addTable(tableBuilder.build());
+ }
+ protoRequests.add(requestBuilder.build());
+ }
+ return protoRequests;
+ }
+
+ /**
+ * Match a CacheTag to this eviction request.
+ * @param cacheTag
+ * @return true if cacheTag matches and the related buffer is eligible for proactive eviction, false otherwise.
+ */
+ public boolean isTagMatch(CacheTag cacheTag) {
+ // TODO: implement this
Review comment:
can you add the link to the jira.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] b-slim commented on a change in pull request #909: HIVE-22821
Posted by GitBox <gi...@apache.org>.
b-slim commented on a change in pull request #909: HIVE-22821
URL: https://github.com/apache/hive/pull/909#discussion_r389754579
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
##########
@@ -221,6 +227,9 @@ public void debugDumpShort(StringBuilder sb) {
metadataCache, dataCache, bufferManagerOrc, conf, cacheMetrics, ioMetrics, tracePool);
this.genericCvp = isEncodeEnabled ? new GenericColumnVectorProducer(
serdeCache, bufferManagerGeneric, conf, cacheMetrics, ioMetrics, tracePool) : null;
+ proactiveEvictionExecutor = Executors.newSingleThreadExecutor(
Review comment:
I do not agree, When you say wait can you explain what that means ?
Whom is waiting and where and why?
Then how spinning up new thread pool will help that if there is contention?
Also you are mentioning HS2I but this code is in the LLAP runner so not sure how to read this.
Finally when you say long time or wait how long you think will take ? i do not see how this is long and do not see what it is blocking.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org