[GitHub] [incubator-druid] jihoonson commented on a change in pull request #8925: Parallel indexing single dim partitions
jihoonson commented on a change in pull request #8925: Parallel indexing single
dim partitions
URL: https://github.com/apache/incubator-druid/pull/8925#discussion_r355106547
##
File path:
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/distribution/PartitionBoundaries.java
##
@@ -20,28 +20,48 @@
package org.apache.druid.indexing.common.task.batch.parallel.distribution;
import com.google.common.collect.ForwardingList;
-import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
+import java.util.stream.Collectors;
/**
- * Convenience wrapper to make code more readable.
+ * List of range partition boundaries.
*/
-public class Partitions extends ForwardingList implements List
+public class PartitionBoundaries extends ForwardingList implements
List
{
private final List delegate;
// For jackson
@SuppressWarnings("unused")
- private Partitions()
+ private PartitionBoundaries()
{
delegate = new ArrayList<>();
}
- public Partitions(String... partitions)
+ /**
+ * @param partitions Elements corresponding to evenly-spaced fractional
ranks of the distribution
+ */
+ public PartitionBoundaries(String... partitions)
{
-delegate = ImmutableList.copyOf(partitions);
+if (partitions.length == 0) {
+ delegate = Collections.emptyList();
+ return;
+}
+
+List partitionBoundaries = Arrays.stream(partitions)
+ .distinct()
+
.collect(Collectors.toCollection(ArrayList::new));
+
+// First partition starts with null (see StringPartitionChunk.isStart())
+partitionBoundaries.set(0, null);
+
+// Last partition ends with null (see StringPartitionChunk.isEnd())
+partitionBoundaries.add(null);
Review comment:
> Previously, I had logic to combine it with the second-to-last partition
when it was small:
> [#8925
(comment)](https://github.com/apache/incubator-druid/pull/8925#discussion_r350539718)
>
> If it's still desired to not have that logic to decide whether to combine
it or not, then it needs to either always be combined or never be combined.
I guess the small partitions are already being always combined by selecting
distinct keys
[here](https://github.com/apache/incubator-druid/pull/8925/files/83ab7a86a24c1eb739c3f6e44974d68b188ab898#diff-57c86134ac6a13111baab12eff55827bR54-R56)?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] qutang1 commented on a change in pull request #8990: Fix double-checked locking in predicate for SelectorDimFilter
qutang1 commented on a change in pull request #8990: Fix double-checked locking
in predicate for SelectorDimFilter
URL: https://github.com/apache/incubator-druid/pull/8990#discussion_r355109998
##
File path:
processing/src/main/java/org/apache/druid/query/filter/SelectorDimFilter.java
##
@@ -71,7 +73,9 @@ public SelectorDimFilter(
)
{
Preconditions.checkArgument(dimension != null, "dimension must not be
null");
-
+this.longPredicate = makeLongPredicateSupplier().get();
Review comment:
Hi Leventov, I am not sure if I understand this case correctly : basically
the original code is saying I want 3 private predicates only be initialized
once when SelectorDimFilter::toFilter() method has been called +
DruidPredicateFactory::makeDoublePredicate() has been called, is it correct? My
question is since DruidPredicateFactory is final(already thread safe), the
whole purpose of the old logic is just not init the predicate twice ,all I need
to do is change them to final...
` @Override
public Filter toFilter()
{
if (extractionFn == null) {
return new SelectorFilter(dimension, value, filterTuning);
} else {
final DruidPredicateFactory predicateFactory = new
DruidPredicateFactory()
{
@Override
public Predicate makeStringPredicate()
{
return Predicates.equalTo(value);
}
@Override
public DruidLongPredicate makeLongPredicate()
{
initLongPredicate();
return longPredicate;
}
@Override
public DruidFloatPredicate makeFloatPredicate()
{
initFloatPredicate();
return floatPredicate;
}
@Override
public DruidDoublePredicate makeDoublePredicate()
{
initDoublePredicate();
return druidDoublePredicate;
}
};
return new DimensionPredicateFilter(dimension, predicateFactory,
extractionFn, filterTuning);
}
}
`
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] qutang1 commented on a change in pull request #8990: Fix double-checked locking in predicate for SelectorDimFilter
qutang1 commented on a change in pull request #8990: Fix double-checked locking
in predicate for SelectorDimFilter
URL: https://github.com/apache/incubator-druid/pull/8990#discussion_r355109998
##
File path:
processing/src/main/java/org/apache/druid/query/filter/SelectorDimFilter.java
##
@@ -71,7 +73,9 @@ public SelectorDimFilter(
)
{
Preconditions.checkArgument(dimension != null, "dimension must not be
null");
-
+this.longPredicate = makeLongPredicateSupplier().get();
Review comment:
Hi Leventov, I am not sure if I understand this case correctly : basically
the original code is saying I want 3 private predicates only be initialized
once when SelectorDimFilter::toFilter() method has been called +
DruidPredicateFactory::makeDoublePredicate() has been called, is it correct? My
question is since DruidPredicateFactory is final(already thread safe), the
whole purpose of the old logic is just not init the predicate twice ,all I need
to do is change 3 predicate fields to final?
` @Override
public Filter toFilter()
{
if (extractionFn == null) {
return new SelectorFilter(dimension, value, filterTuning);
} else {
final DruidPredicateFactory predicateFactory = new
DruidPredicateFactory()
{
@Override
public Predicate makeStringPredicate()
{
return Predicates.equalTo(value);
}
@Override
public DruidLongPredicate makeLongPredicate()
{
initLongPredicate();
return longPredicate;
}
@Override
public DruidFloatPredicate makeFloatPredicate()
{
initFloatPredicate();
return floatPredicate;
}
@Override
public DruidDoublePredicate makeDoublePredicate()
{
initDoublePredicate();
return druidDoublePredicate;
}
};
return new DimensionPredicateFilter(dimension, predicateFactory,
extractionFn, filterTuning);
}
}
`
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] qutang1 commented on a change in pull request #8990: Fix double-checked locking in predicate for SelectorDimFilter
qutang1 commented on a change in pull request #8990: Fix double-checked locking
in predicate for SelectorDimFilter
URL: https://github.com/apache/incubator-druid/pull/8990#discussion_r355109998
##
File path:
processing/src/main/java/org/apache/druid/query/filter/SelectorDimFilter.java
##
@@ -71,7 +73,9 @@ public SelectorDimFilter(
)
{
Preconditions.checkArgument(dimension != null, "dimension must not be
null");
-
+this.longPredicate = makeLongPredicateSupplier().get();
Review comment:
Hi Leventov, I am not sure if I understand this case correctly : basically
the original code is saying I want 3 private predicates only be initialized
once when SelectorDimFilter::toFilter() method has been called +
DruidPredicateFactory::makeDoublePredicate() has been called, is it correct? My
question is since DruidPredicateFactory is final(already thread safe), the
whole purpose of the old logic is just not init the predicate twice ,there's no
thread safety issue has been involved, all I need to do is change 3 predicate
fields to final?
` @Override
public Filter toFilter()
{
if (extractionFn == null) {
return new SelectorFilter(dimension, value, filterTuning);
} else {
final DruidPredicateFactory predicateFactory = new
DruidPredicateFactory()
{
@Override
public Predicate makeStringPredicate()
{
return Predicates.equalTo(value);
}
@Override
public DruidLongPredicate makeLongPredicate()
{
initLongPredicate();
return longPredicate;
}
@Override
public DruidFloatPredicate makeFloatPredicate()
{
initFloatPredicate();
return floatPredicate;
}
@Override
public DruidDoublePredicate makeDoublePredicate()
{
initDoublePredicate();
return druidDoublePredicate;
}
};
return new DimensionPredicateFilter(dimension, predicateFactory,
extractionFn, filterTuning);
}
}
`
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8990: Fix double-checked locking in predicate for SelectorDimFilter
leventov commented on a change in pull request #8990: Fix double-checked
locking in predicate for SelectorDimFilter
URL: https://github.com/apache/incubator-druid/pull/8990#discussion_r355115243
##
File path:
processing/src/main/java/org/apache/druid/query/filter/SelectorDimFilter.java
##
@@ -71,7 +73,9 @@ public SelectorDimFilter(
)
{
Preconditions.checkArgument(dimension != null, "dimension must not be
null");
-
+this.longPredicate = makeLongPredicateSupplier().get();
Review comment:
> DruidPredicateFactory is final(already thread safe)
DruidPredicateFactory is not thread safe, as far as I can tell.
You cannot initialize predicates eagerly since it may not be possible: e. g.
if the `value` is `"1.0"`, it will not be parseable as long in
`createLongPredicate()`. The method will not throw because it uses
`tryParseLong()`, but nevertheless this is fragile.
The problem with the current code is that it allows spurious null returns of
predicates due to double-read of a non-volatile field, as described
[here](https://github.com/code-review-checklists/java-concurrency#safe-local-dcl),
see
[here](https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-hb-actual)
for more theoretical foundations.
You can fix this by either applying proper double-checked locking, or using
[*racy single check
initialization*](https://github.com/code-review-checklists/java-concurrency#lazy-init-benign-race).
BTW, also makes sense to cache `makeStringPredicate()`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on issue #8848: Refactor DirectDruidClient, extract QueryContextInputStreamResponseHandler
leventov commented on issue #8848: Refactor DirectDruidClient, extract QueryContextInputStreamResponseHandler URL: https://github.com/apache/incubator-druid/pull/8848#issuecomment-562839706 Inspection build still fails: https://teamcity.jetbrains.com/viewLog.html?buildId=2665308&tab=Inspection&buildTypeId=OpenSourceProjects_Druid_InspectionsPullRequests 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r355115743
##
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/RemoteTaskRunner.java
##
@@ -350,6 +350,7 @@ public void stop()
return;
}
try {
+ monitorSyncHandler.shutdown();
Review comment:
These two things must be closed using the `closer` below
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r355115822 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java ## @@ -219,6 +219,7 @@ public void stop() tasks.clear(); taskFutures.clear(); active = false; + statusHandler.shutdownNow(); Review comment: Please shutdown these three Executors using a `Closer` 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback)
leventov commented on a change in pull request #8809: Prohibit Futures.addCallback(Future, Callback) URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r355116113 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java ## @@ -219,6 +221,7 @@ private final List> handOffWaitList = new ArrayList<>(); private final LockGranularity lockGranularityToUse; + private final ExecutorService sequencePersistExecutor; Review comment: In classes that don't have `close()`/`stop()` methods yet, like `SeekableStreamIndexTaskRunner`, they should be introduced if you want to manage an executor. If the class is pretty ephemeral (e. g. many instances of this class routinely start and stop), it should be strongly preferred to create an executor one level up the stack and reuse it for all instances of the class (or all instances of the class within a certain domain e. g. all `SeekableStreamIndexTaskRunner`s associated with a single datasource, or something like that). Note again, I don't actually tell you that this class `SeekableStreamIndexTaskRunner` falls into this category - please figure it out yourself, or consult the maintainers of this subsystem. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on issue #8080: Update master TeamCity CI to use IntelliJ 2019.3 in 2020
leventov commented on issue #8080: Update master TeamCity CI to use IntelliJ 2019.3 in 2020 URL: https://github.com/apache/incubator-druid/issues/8080#issuecomment-562842453 This has happened, the new version of IntelliJ is being tested: https://teamcity.jetbrains.com/viewLog.html?buildId=2665645&buildTypeId=OpenSourceProjects_Druid_Inspections 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] Doslin commented on issue #7918: Is there an example of druid with mybatis?
Doslin commented on issue #7918: Is there an example of druid with mybatis? URL: https://github.com/apache/incubator-druid/issues/7918#issuecomment-562845649 > Is this still and issue in 0.16? yes ,This problem still exists 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] fjy merged pull request #8998: update dump-segment docs so example command works
fjy merged pull request #8998: update dump-segment docs so example command works URL: https://github.com/apache/incubator-druid/pull/8998 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[incubator-druid] branch master updated: update dump-segment docs so example command works (#8998)
This is an automated email from the ASF dual-hosted git repository. fjy pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-druid.git The following commit(s) were added to refs/heads/master by this push: new 441515c update dump-segment docs so example command works (#8998) 441515c is described below commit 441515cb50e67dcb5d485acdd4e74c766a363ba2 Author: Clint Wylie AuthorDate: Sat Dec 7 06:36:46 2019 -0800 update dump-segment docs so example command works (#8998) * update dump-segment docs so example command works * not everyone uses bash --- docs/operations/dump-segment.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/operations/dump-segment.md b/docs/operations/dump-segment.md index 3e3c529..6da2415 100644 --- a/docs/operations/dump-segment.md +++ b/docs/operations/dump-segment.md @@ -30,7 +30,8 @@ complex metric values may not be complete. To run the tool, point it at a segment directory and provide a file for writing output: ``` -java -classpath "/my/druid/lib/*" org.apache.druid.cli.Main tools dump-segment \ +java -classpath "/my/druid/lib/*" -Ddruid.extensions.loadList="[]" org.apache.druid.cli.Main \ + tools dump-segment \ --directory /home/druid/path/to/segment/ \ --out /home/druid/output.txt ``` - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] fjy closed issue #8997: Could not able to use segment-dump tool - SQLAuditManagerConfig was already configured error
fjy closed issue #8997: Could not able to use segment-dump tool - SQLAuditManagerConfig was already configured error URL: https://github.com/apache/incubator-druid/issues/8997 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] himanshushukla254 closed issue #8966: Issue while updating the data in Datasource
himanshushukla254 closed issue #8966: Issue while updating the data in Datasource URL: https://github.com/apache/incubator-druid/issues/8966 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] Doslin closed issue #8973: Why generated __subquery__ and use DruidOuterQueryRel
Doslin closed issue #8973: Why generated __subquery__ and use DruidOuterQueryRel URL: https://github.com/apache/incubator-druid/issues/8973 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
