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 2021/12/15 12:43:46 UTC
[GitHub] [hive] veghlaci05 opened a new pull request #2879: HIVE-25801
veghlaci05 opened a new pull request #2879:
URL: https://github.com/apache/hive/pull/2879
### What changes were proposed in this pull request?
This PR fixes the issue that the statistics gathering for Query based compactions are not honouring the custom queue settings. It also deprecates the hive.mr.compactor.gather.stats setting which is honoured only by the MR based compactor, and introduces the hive.compactor.gather.stats setting, which is honoured by both MR and Query based compactors. The values of the two settings are combined with logical OR in case of MR based compaction. The stats gathering also changed since there is no need for column based statistics gathering in case of compaction, the basic statistics should be enough.
### Why are the changes needed?
hive.compactor.job.queue config limits resources available for compaction, so users can limit the effects of compaction on the cluster. However this setting does not affect stats collection which uses Driver.
HIVE-25595 is addressing the above issue for MR-based compaction. We need to incorporate the same thing for the Query-based compaction.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
It was tested using the already existing automated tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781812910
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java
##########
@@ -506,10 +478,23 @@ protected Boolean findNextCompactionAndExecute(boolean computeStats) {
task (currently we're using Tez split grouping).
*/
QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
+ boolean generic_stats = conf.getBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS);
Review comment:
I moved it back
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r770721271
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java
##########
@@ -506,10 +478,23 @@ protected Boolean findNextCompactionAndExecute(boolean computeStats) {
task (currently we're using Tez split grouping).
*/
QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
+ boolean generic_stats = conf.getBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS);
Review comment:
Why did you move the config evaluation inside of the compaction loop?
Please use camelCase naming convention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781177444
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java
##########
@@ -505,19 +472,29 @@ protected Boolean findNextCompactionAndExecute(boolean computeStats) {
task (currently we're using Tez split grouping).
*/
QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
+ boolean computeStats = (queryCompactor == null && (collectMrStats || collectGenericStats)) || collectGenericStats;
Review comment:
you can simplify
````
computeStats = (queryCompactor == null && collectMrStats) || collectGenericStats;
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781814295
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java
##########
@@ -505,19 +472,29 @@ protected Boolean findNextCompactionAndExecute(boolean computeStats) {
task (currently we're using Tez split grouping).
*/
QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
+ boolean computeStats = (queryCompactor == null && (collectMrStats || collectGenericStats)) || collectGenericStats;
Review comment:
Applied
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781814842
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java
##########
@@ -1297,18 +1297,20 @@ public static FileStatus validateTargetDir(Path path, Configuration conf) throws
}
// the api that finds the jar being used by this class on disk
- public String getExecJarPathLocal(Configuration configuration) throws URISyntaxException {
- // returns the location on disc of the jar of this class.
-
- URI uri = DagUtils.class.getProtectionDomain().getCodeSource().getLocation().toURI();
- if (configuration.getBoolean(ConfVars.HIVE_IN_TEST_IDE.varname, false)) {
- if (new File(uri.getPath()).isDirectory()) {
- // IDE support for running tez jobs
- uri = createEmptyArchive();
+ public String getExecJarPathLocal(Configuration configuration) {
+ try {
+ // returns the location on disc of the jar of this class.
+ String uri = DagUtils.class.getProtectionDomain().getCodeSource().getLocation().toURI().toString();
+ if (uri.endsWith(".jar")) {
Review comment:
Should not, but modified the catch block to Exception class, just to make sure no exception is thrown from this method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r770724374
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java
##########
@@ -506,10 +478,23 @@ protected Boolean findNextCompactionAndExecute(boolean computeStats) {
task (currently we're using Tez split grouping).
*/
QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
+ boolean generic_stats = conf.getBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS);
+ boolean mr_stats = conf.getBoolVar(HiveConf.ConfVars.HIVE_MR_COMPACTOR_GATHER_STATS);
+ boolean computeStats = (queryCompactor == null && (mr_stats || generic_stats)) || generic_stats;
+
+ LOG.info("Starting " + ci.type.toString() + " compaction for " + ci.getFullPartitionName() + " in " +
+ compactionTxn + " with compute stats set to " + computeStats);
+
+ StatsUpdater su = null;
+ if (computeStats) {
+ su = StatsUpdater.init(ci, conf, runJobAsSelf(ci.runAs) ? ci.runAs : t.getOwner(),
Review comment:
Q. can we refactor StatsUpdater to a service utility that just submits the compute stats query and not generate a separate instance per each compaction candidate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ merged pull request #2879: HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ merged pull request #2879:
URL: https://github.com/apache/hive/pull/2879
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r782809575
##########
File path: ql/src/test/org/apache/hadoop/hive/ql/TestTxnNoBuckets.java
##########
@@ -833,10 +833,6 @@ public void testCompactStatsGather() throws Exception {
Assert.assertEquals("Unexpected number of compactions in history", 1, resp.getCompactsSize());
Assert.assertEquals("Unexpected 0 compaction state", TxnStore.CLEANING_RESPONSE, resp.getCompacts().get(0).getState());
Assert.assertTrue(resp.getCompacts().get(0).getHadoopJobId().startsWith("job_local"));
-
- //now check that stats were updated
- map = hms.getPartitionColumnStatistics("default","T", partNames, colNames, Constants.HIVE_ENGINE);
Review comment:
Modified the test to check basic stats only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781174986
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java
##########
@@ -87,17 +87,18 @@
private String workerName;
// TODO: this doesn't check if compaction is already running (even though Initiator does but we
- // don't go through Initiator for user initiated compactions)
+ // ,mdon't go through Initiator for user initiated compactions)
Review comment:
nit, typo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781228088
##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java
##########
@@ -68,10 +59,16 @@
import org.junit.Assert;
import org.junit.Test;
-import static org.apache.hadoop.hive.ql.txn.compactor.TestCompactor.executeStatementOnDriver;
+import java.io.IOException;
Review comment:
There were some unused imports and I used Intellij Idea;s 'organize imports' command which orders and groups the imports as well on top of removing the unused ones.
##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java
##########
@@ -68,10 +59,16 @@
import org.junit.Assert;
import org.junit.Test;
-import static org.apache.hadoop.hive.ql.txn.compactor.TestCompactor.executeStatementOnDriver;
+import java.io.IOException;
Review comment:
There were some unused imports and I used Intellij Idea,s 'organize imports' command which orders and groups the imports as well on top of removing the unused ones.
##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java
##########
@@ -68,10 +59,16 @@
import org.junit.Assert;
import org.junit.Test;
-import static org.apache.hadoop.hive.ql.txn.compactor.TestCompactor.executeStatementOnDriver;
+import java.io.IOException;
Review comment:
There were some unused imports and I used Intellij Idea's 'organize imports' command which orders and groups the imports as well on top of removing the unused ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r782193090
##########
File path: ql/src/test/org/apache/hadoop/hive/ql/TestTxnNoBuckets.java
##########
@@ -833,10 +833,6 @@ public void testCompactStatsGather() throws Exception {
Assert.assertEquals("Unexpected number of compactions in history", 1, resp.getCompactsSize());
Assert.assertEquals("Unexpected 0 compaction state", TxnStore.CLEANING_RESPONSE, resp.getCompacts().get(0).getState());
Assert.assertTrue(resp.getCompacts().get(0).getHadoopJobId().startsWith("job_local"));
-
- //now check that stats were updated
- map = hms.getPartitionColumnStatistics("default","T", partNames, colNames, Constants.HIVE_ENGINE);
Review comment:
should we check basic statistics here instead on colStat?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r782189473
##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCompactor.java
##########
@@ -90,7 +67,25 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import com.google.common.collect.Lists;
+import java.io.File;
Review comment:
imports structure is not restored
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r782198917
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -3257,11 +3257,23 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
HIVE_COMPACTOR_WAIT_TIMEOUT("hive.compactor.wait.timeout", 300000L, "Time out in "
+ "milliseconds for blocking compaction. It's value has to be higher than 2000 milliseconds. "),
- HIVE_MR_COMPACTOR_GATHER_STATS("hive.mr.compactor.gather.stats", true, "If set to true MAJOR compaction " +
+ /**
+ * @deprecated This config value is honoured by the MR based compaction only.
+ * Use the {@link HiveConf.ConfVars#HIVE_COMPACTOR_GATHER_STATS}
+ * config instead which is honoured by both the MR and Query based compaction.
+ */
+ @Deprecated
+ HIVE_MR_COMPACTOR_GATHER_STATS("hive.mr.compactor.gather.stats", false, "If set to true MAJOR compaction " +
"will gather stats if there are stats already associated with the table/partition.\n" +
"Turn this off to save some resources and the stats are not used anyway.\n" +
"Works only for MR based compaction, CRUD based compaction uses hive.stats.autogather."),
+ HIVE_COMPACTOR_GATHER_STATS("hive.compactor.gather.stats", true, "If set to true MAJOR compaction " +
Review comment:
Do we re-calculate stats only in case of MAJOR compaction?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781814139
##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java
##########
@@ -68,10 +59,16 @@
import org.junit.Assert;
import org.junit.Test;
-import static org.apache.hadoop.hive.ql.txn.compactor.TestCompactor.executeStatementOnDriver;
+import java.io.IOException;
Review comment:
Reverted the reorder, only removed the unused ones
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781173357
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java
##########
@@ -1297,18 +1297,20 @@ public static FileStatus validateTargetDir(Path path, Configuration conf) throws
}
// the api that finds the jar being used by this class on disk
- public String getExecJarPathLocal(Configuration configuration) throws URISyntaxException {
- // returns the location on disc of the jar of this class.
-
- URI uri = DagUtils.class.getProtectionDomain().getCodeSource().getLocation().toURI();
- if (configuration.getBoolean(ConfVars.HIVE_IN_TEST_IDE.varname, false)) {
- if (new File(uri.getPath()).isDirectory()) {
- // IDE support for running tez jobs
- uri = createEmptyArchive();
+ public String getExecJarPathLocal(Configuration configuration) {
+ try {
+ // returns the location on disc of the jar of this class.
+ String uri = DagUtils.class.getProtectionDomain().getCodeSource().getLocation().toURI().toString();
+ if (uri.endsWith(".jar")) {
Review comment:
can we get NPE here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781174435
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/QueryCompactor.java
##########
@@ -56,6 +56,7 @@
* @param storageDescriptor this is the resolved storage descriptor
* @param writeIds valid write IDs used to filter rows while they're being read for compaction
* @param compactionInfo provides info about the type of compaction
+ * @param dir provides ACID directory layout information
Review comment:
redundant
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r770700352
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java
##########
@@ -1299,6 +1299,10 @@ public static FileStatus validateTargetDir(Path path, Configuration conf) throws
// the api that finds the jar being used by this class on disk
public String getExecJarPathLocal(Configuration configuration) throws URISyntaxException {
// returns the location on disc of the jar of this class.
+ String jar = configuration.get(ConfVars.HIVEJAR.varname);
+ if (!StringUtils.isBlank(jar)) {
+ return jar;
Review comment:
why is this change 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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r770713726
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/QueryCompactor.java
##########
@@ -137,6 +140,10 @@ void runCompactionQueries(HiveConf conf, String tmpTableName, StorageDescriptor
DriverUtils.runOnDriver(conf, user, sessionState, query, writeIds, compactorTxnId);
}
commitCompaction(storageDescriptor.getLocation(), tmpTableName, conf, writeIds, compactorTxnId);
+
+ if (su != null) {
Review comment:
why not call it from Worker, on compaction txn commit?
````
msc.commitTxn(txnId);
status = TxnStatus.COMMITTED;
su.gatherStats()
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r782193090
##########
File path: ql/src/test/org/apache/hadoop/hive/ql/TestTxnNoBuckets.java
##########
@@ -833,10 +833,6 @@ public void testCompactStatsGather() throws Exception {
Assert.assertEquals("Unexpected number of compactions in history", 1, resp.getCompactsSize());
Assert.assertEquals("Unexpected 0 compaction state", TxnStore.CLEANING_RESPONSE, resp.getCompacts().get(0).getState());
Assert.assertTrue(resp.getCompacts().get(0).getHadoopJobId().startsWith("job_local"));
-
- //now check that stats were updated
- map = hms.getPartitionColumnStatistics("default","T", partNames, colNames, Constants.HIVE_ENGINE);
Review comment:
should we check basic statistics here instead of colStat?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r781226226
##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java
##########
@@ -68,10 +59,16 @@
import org.junit.Assert;
import org.junit.Test;
-import static org.apache.hadoop.hive.ql.txn.compactor.TestCompactor.executeStatementOnDriver;
+import java.io.IOException;
Review comment:
why is the import reorder?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r782189125
##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCompactor.java
##########
@@ -335,153 +330,144 @@ public void schemaEvolutionAddColDynamicPartitioningUpdate() throws Exception {
}
/**
- * After each major compaction, stats need to be updated on each column of the
- * table/partition which previously had stats.
- * 1. create a bucketed ORC backed table (Orc is currently required by ACID)
- * 2. populate 2 partitions with data
+ * After each major compaction, stats need to be updated on the table
+ * 1. create a partitioned ORC backed table (Orc is currently required by ACID)
+ * 2. populate with data
* 3. compute stats
- * 4. insert some data into the table using StreamingAPI
- * 5. Trigger major compaction (which should update stats)
- * 6. check that stats have been updated
+ * 4. Trigger major compaction on one of the partitions (which should update stats)
+ * 5. check that stats have been updated for that partition only
*
* @throws Exception todo:
- * 2. add non-partitioned test
* 4. add a test with sorted table?
*/
@Test
public void testStatsAfterCompactionPartTbl() throws Exception {
//as of (8/27/2014) Hive 0.14, ACID/Orc requires HiveInputFormat
+ String dbName = "default";
String tblName = "compaction_test";
- String tblNameStg = tblName + "_stg";
- List<String> colNames = Arrays.asList("a", "b");
executeStatementOnDriver("drop table if exists " + tblName, driver);
- executeStatementOnDriver("drop table if exists " + tblNameStg, driver);
executeStatementOnDriver("CREATE TABLE " + tblName + "(a INT, b STRING) " +
" PARTITIONED BY(bkt INT)" +
" CLUSTERED BY(a) INTO 4 BUCKETS" + //currently ACID requires table to be bucketed
" STORED AS ORC TBLPROPERTIES ('transactional'='true')", driver);
- executeStatementOnDriver("CREATE EXTERNAL TABLE " + tblNameStg + "(a INT, b STRING)" +
- " ROW FORMAT DELIMITED FIELDS TERMINATED BY '\\t' LINES TERMINATED BY '\\n'" +
- " STORED AS TEXTFILE" +
- " LOCATION '" + stagingFolder.newFolder().toURI().getPath() + "'", driver);
-
- executeStatementOnDriver("load data local inpath '" + BASIC_FILE_NAME +
- "' overwrite into table " + tblNameStg, driver);
- execSelectAndDumpData("select * from " + tblNameStg, driver, "Dumping data for " +
- tblNameStg + " after load:");
- executeStatementOnDriver("FROM " + tblNameStg +
- " INSERT INTO TABLE " + tblName + " PARTITION(bkt=0) " +
- "SELECT a, b where a < 2", driver);
- executeStatementOnDriver("FROM " + tblNameStg +
- " INSERT INTO TABLE " + tblName + " PARTITION(bkt=1) " +
- "SELECT a, b where a >= 2", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tblName + " PARTITION(bkt=0)" +
+ " values(55, 'London')", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tblName + " PARTITION(bkt=0)" +
+ " values(56, 'Paris')", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tblName + " PARTITION(bkt=1)" +
+ " values(57, 'Budapest')", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tblName + " PARTITION(bkt=1)" +
+ " values(58, 'Milano')", driver);
execSelectAndDumpData("select * from " + tblName, driver, "Dumping data for " +
tblName + " after load:");
TxnStore txnHandler = TxnUtils.getTxnStore(conf);
- CompactionInfo ci = new CompactionInfo("default", tblName, "bkt=0", CompactionType.MAJOR);
- Table table = msClient.getTable("default", tblName);
- LOG.debug("List of stats columns before analyze Part1: " + txnHandler.findColumnsWithStats(ci));
- Worker.StatsUpdater su = Worker.StatsUpdater.init(ci, colNames, conf,
- System.getProperty("user.name"), CompactorUtil.getCompactorJobQueueName(conf, ci, table));
- su.gatherStats();//compute stats before compaction
- LOG.debug("List of stats columns after analyze Part1: " + txnHandler.findColumnsWithStats(ci));
-
- CompactionInfo ciPart2 = new CompactionInfo("default", tblName, "bkt=1", CompactionType.MAJOR);
- LOG.debug("List of stats columns before analyze Part2: " + txnHandler.findColumnsWithStats(ci));
- su = Worker.StatsUpdater.init(ciPart2, colNames, conf, System.getProperty("user.name"),
- CompactorUtil.getCompactorJobQueueName(conf, ciPart2, table));
- su.gatherStats();//compute stats before compaction
- LOG.debug("List of stats columns after analyze Part2: " + txnHandler.findColumnsWithStats(ci));
-
- //now make sure we get the stats we expect for partition we are going to add data to later
- Map<String, List<ColumnStatisticsObj>> stats = msClient.getPartitionColumnStatistics(ci.dbname,
- ci.tableName, Arrays.asList(ci.partName), colNames, Constants.HIVE_ENGINE);
- List<ColumnStatisticsObj> colStats = stats.get(ci.partName);
- assertNotNull("No stats found for partition " + ci.partName, colStats);
- Assert.assertEquals("Expected column 'a' at index 0", "a", colStats.get(0).getColName());
- Assert.assertEquals("Expected column 'b' at index 1", "b", colStats.get(1).getColName());
- LongColumnStatsData colAStats = colStats.get(0).getStatsData().getLongStats();
- Assert.assertEquals("lowValue a", 1, colAStats.getLowValue());
- Assert.assertEquals("highValue a", 1, colAStats.getHighValue());
- Assert.assertEquals("numNulls a", 0, colAStats.getNumNulls());
- Assert.assertEquals("numNdv a", 1, colAStats.getNumDVs());
- StringColumnStatsData colBStats = colStats.get(1).getStatsData().getStringStats();
- Assert.assertEquals("maxColLen b", 3, colBStats.getMaxColLen());
- Assert.assertEquals("avgColLen b", 3.0, colBStats.getAvgColLen(), 0.01);
- Assert.assertEquals("numNulls b", 0, colBStats.getNumNulls());
- Assert.assertEquals("nunDVs", 3, colBStats.getNumDVs());
-
- //now save stats for partition we won't modify
- stats = msClient.getPartitionColumnStatistics(ciPart2.dbname,
- ciPart2.tableName, Arrays.asList(ciPart2.partName), colNames, Constants.HIVE_ENGINE);
- colStats = stats.get(ciPart2.partName);
- LongColumnStatsData colAStatsPart2 = colStats.get(0).getStatsData().getLongStats();
- StringColumnStatsData colBStatsPart2 = colStats.get(1).getStatsData().getStringStats();
+ Table table = msClient.getTable(dbName, tblName);
+
+ //compute stats before compaction
+ CompactionInfo ci = new CompactionInfo(dbName, tblName, "bkt=0", CompactionType.MAJOR);
+ Worker.StatsUpdater.gatherStats(ci, conf,
+ System.getProperty("user.name"), CompactorUtil.getCompactorJobQueueName(conf, ci, table));
+ ci = new CompactionInfo(dbName, tblName, "bkt=1", CompactionType.MAJOR);
+ Worker.StatsUpdater.gatherStats(ci, conf,
+ System.getProperty("user.name"), CompactorUtil.getCompactorJobQueueName(conf, ci, table));
+
+ //Check basic stats are collected
+ org.apache.hadoop.hive.ql.metadata.Table hiveTable = Hive.get().getTable(tblName);
+ Map<String, String> parameters = Hive.get().getPartitions(hiveTable).get(0).getParameters();
Review comment:
should we check both partitions as you are loading data into both?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a change in pull request #2879: [WIP]HIVE-25801: Custom queue setting is not honoured by Query based compaction StatsUpdater
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on a change in pull request #2879:
URL: https://github.com/apache/hive/pull/2879#discussion_r782214222
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -3257,11 +3257,23 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
HIVE_COMPACTOR_WAIT_TIMEOUT("hive.compactor.wait.timeout", 300000L, "Time out in "
+ "milliseconds for blocking compaction. It's value has to be higher than 2000 milliseconds. "),
- HIVE_MR_COMPACTOR_GATHER_STATS("hive.mr.compactor.gather.stats", true, "If set to true MAJOR compaction " +
+ /**
+ * @deprecated This config value is honoured by the MR based compaction only.
+ * Use the {@link HiveConf.ConfVars#HIVE_COMPACTOR_GATHER_STATS}
+ * config instead which is honoured by both the MR and Query based compaction.
+ */
+ @Deprecated
+ HIVE_MR_COMPACTOR_GATHER_STATS("hive.mr.compactor.gather.stats", false, "If set to true MAJOR compaction " +
"will gather stats if there are stats already associated with the table/partition.\n" +
"Turn this off to save some resources and the stats are not used anyway.\n" +
"Works only for MR based compaction, CRUD based compaction uses hive.stats.autogather."),
+ HIVE_COMPACTOR_GATHER_STATS("hive.compactor.gather.stats", true, "If set to true MAJOR compaction " +
Review comment:
Yes, this is the current behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org