date:20191105

[GitHub] [incubator-druid] Fokko edited a comment on issue #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

Fokko edited a comment on issue #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419#issuecomment-550016704
 
 
   `scrooge-maven-plugin` is already using Thrift 0.10.0. The old version 
`4.11.0` of `scrooge-maven-plugin` is indeed using `libthrift-0.5.0-1 jar`. 
I've rebased onto master.
   
   Why is this a development blocker?


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] scottbelden opened a new issue #8827: Failure to ingest local file when running through the tutorial with the docker cluster

2019-11-05 Thread GitBox

scottbelden opened a new issue #8827: Failure to ingest local file when running 
through the tutorial with the docker cluster
URL: https://github.com/apache/incubator-druid/issues/8827
 
 
   ### Affected Version
   
   0.16.0-incubating
   
   ### Description
   
   I am using the docker-compose file located 
[here](https://github.com/apache/incubator-druid/blob/master/distribution/docker/docker-compose.yml)
 to start a druid cluster. I modified the compose file to specify the 
`0.16.0-incubating` tag for the druid images.
   
   I am trying to follow the tutorial to [ingest a local 
file](https://druid.apache.org/docs/latest/tutorials/tutorial-batch.html) but 
after submitting the job it never succeeds and instead always fails.
   
   Below are the docker-compose logs for the cluster from the time I submitted 
the task to the time that it failed:
   
   ```overlord | 2019-11-05T22:54:22,032 INFO [qtp2107873140-89] 
org.apache.druid.indexing.overlord.MetadataTaskStorage - Inserting task 
index_parallel_wikiticker-2015-09-12-sampled_2019-11-05T22:54:22.026Z with 
status: 
TaskStatus{id=index_parallel_wikiticker-2015-09-12-sampled_2019-11-05T22:54:22.026Z,
 status=RUNNING, duration=-1, errorMsg=null}
   overlord | 2019-11-05T22:54:22,055 INFO [qtp2107873140-89] 
org.apache.druid.indexing.overlord.TaskLockbox - Adding 
task[index_parallel_wikiticker-2015-09-12-sampled_2019-11-05T22:54:22.026Z] to 
activeTasks
   overlord | 2019-11-05T22:54:22,055 INFO [TaskQueue-Manager] 
org.apache.druid.indexing.common.task.AbstractBatchIndexTask - 
[forceTimeChunkLock] is set to true in task context. Use timeChunk lock
   overlord | 2019-11-05T22:54:22,056 INFO [TaskQueue-Manager] 
org.apache.druid.indexing.overlord.TaskQueue - Asking taskRunner to run: 
index_parallel_wikiticker-2015-09-12-sampled_2019-11-05T22:54:22.026Z
   overlord | 2019-11-05T22:54:22,056 INFO [TaskQueue-Manager] 
org.apache.druid.indexing.overlord.RemoteTaskRunner - Added pending task 
index_parallel_wikiticker-2015-09-12-sampled_2019-11-05T22:54:22.026Z
   overlord | 2019-11-05T22:54:22,058 INFO [rtr-pending-tasks-runner-0] 
org.apache.druid.indexing.overlord.RemoteTaskRunner - Coordinator asking 
Worker[172.19.0.6:8091] to add 
task[index_parallel_wikiticker-2015-09-12-sampled_2019-11-05T22:54:22.026Z]
   overlord | 2019-11-05T22:54:22,061 DEBUG [qtp2107873140-89] 
org.apache.druid.jetty.RequestLog - 172.19.0.7 POST 
//172.19.0.4:8081/druid/indexer/v1/task HTTP/1.1
   router   | 2019-11-05T22:54:22,063 DEBUG 
[AsyncManagementForwardingServlet-2b1cd7bc-177] 
org.apache.druid.jetty.RequestLog - 172.19.0.1 POST 
//localhost:4008/druid/indexer/v1/task HTTP/1.1
   overlord | 2019-11-05T22:54:22,076 INFO [rtr-pending-tasks-runner-0] 
org.apache.druid.indexing.overlord.RemoteTaskRunner - Task 
index_parallel_wikiticker-2015-09-12-sampled_2019-11-05T22:54:22.026Z switched 
from pending to running (on [172.19.0.6:8091])
   overlord | 2019-11-05T22:54:22,077 INFO [rtr-pending-tasks-runner-0] 
org.apache.druid.indexing.overlord.TaskRunnerUtils - Task 
[index_parallel_wikiticker-2015-09-12-sampled_2019-11-05T22:54:22.026Z] status 
changed to [RUNNING].
   middlemanager| 2019-11-05T22:54:22,092 INFO 
[WorkerTaskManager-NoticeHandler] 
org.apache.druid.indexing.worker.WorkerTaskManager - 
Task[index_parallel_wikiticker-2015-09-12-sampled_2019-11-05T22:54:22.026Z] 
started.
   middlemanager| 2019-11-05T22:54:22,095 INFO [forking-task-runner-1] 
org.apache.druid.indexing.overlord.ForkingTaskRunner - Running command: java 
-cp

[GitHub] [incubator-druid] SEKIRO-J commented on a change in pull request #8748: Use earliest offset on kafka newly discovered partitions

2019-11-05 Thread GitBox

SEKIRO-J commented on a change in pull request #8748: Use earliest offset on 
kafka newly discovered partitions
URL: https://github.com/apache/incubator-druid/pull/8748#discussion_r342773057
 
 

 ##
 File path: 
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
 ##
 @@ -259,4 +260,36 @@ private static void wrapExceptions(Runnable runnable)
   return null;
 });
   }
+
+  @VisibleForTesting
+  private KafkaConsumer getKafkaConsumer(Map 
consumerConfigs)
+  {
+final Properties props = new Properties();
+addConsumerPropertiesFromConfig(props, sortingMapper, consumerProperties);
+props.putAll(consumerConfigs);
+
+ClassLoader currCtxCl = Thread.currentThread().getContextClassLoader();
+try {
+  
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
+  Deserializer keyDeserializerObject = getKafkaDeserializer(props, 
"key.deserializer");
+  Deserializer valueDeserializerObject = getKafkaDeserializer(props, 
"value.deserializer");
+
+  return new KafkaConsumer<>(props, keyDeserializerObject, 
valueDeserializerObject);
+}
+finally {
+  Thread.currentThread().setContextClassLoader(currCtxCl);
+}
+  }
+
+  @VisibleForTesting
+  public KafkaRecordSupplier(
+  Map consumerProperties,
+  ObjectMapper sortingMapper,
+  Map consumerConfigs
+  )
+  {
+this.consumerProperties = consumerProperties;
+this.sortingMapper = sortingMapper;
+this.consumer = getKafkaConsumer(consumerConfigs);
+  }
 
 Review comment:
   `getKafkaConsumer()` is a private method. And it depends on other parameters.


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] himanshug edited a comment on issue #8578: parallel broker merges on fork join pool

2019-11-05 Thread GitBox

himanshug edited a comment on issue #8578: parallel broker merges on fork join 
pool
URL: https://github.com/apache/incubator-druid/pull/8578#issuecomment-549904438
 
 
   @clintropolis thanks for all the benchmarks, I haven't had the opportunity 
to look at the new developments yet but get back to reviewing this week.
   
   one thing, I am not sure if taken care or not, many people run the druid 
processes inside docker containers where 
Runtime.getRuntime().availableProcessors() returns the available processors 
from host and not from the "container". ( 
https://bugs.openjdk.java.net/browse/JDK-8140793 , I think it has been changed 
in jdk10) .  Given the sensitivity of performance to availableProcessors() 
returned value, it might be good to make that area a bit configurable if not 
already. I will hopefully offer more specific suggestion when reviewing again.


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] SEKIRO-J commented on a change in pull request #8748: Use earliest offset on kafka newly discovered partitions

2019-11-05 Thread GitBox

SEKIRO-J commented on a change in pull request #8748: Use earliest offset on 
kafka newly discovered partitions
URL: https://github.com/apache/incubator-druid/pull/8748#discussion_r342776259
 
 

 ##
 File path: 
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
 ##
 @@ -259,4 +260,36 @@ private static void wrapExceptions(Runnable runnable)
   return null;
 });
   }
+
+  @VisibleForTesting
+  private KafkaConsumer getKafkaConsumer(Map 
consumerConfigs)
+  {
+final Properties props = new Properties();
+addConsumerPropertiesFromConfig(props, sortingMapper, consumerProperties);
+props.putAll(consumerConfigs);
+
+ClassLoader currCtxCl = Thread.currentThread().getContextClassLoader();
+try {
+  
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
+  Deserializer keyDeserializerObject = getKafkaDeserializer(props, 
"key.deserializer");
+  Deserializer valueDeserializerObject = getKafkaDeserializer(props, 
"value.deserializer");
+
+  return new KafkaConsumer<>(props, keyDeserializerObject, 
valueDeserializerObject);
+}
+finally {
+  Thread.currentThread().setContextClassLoader(currCtxCl);
+}
+  }
+
+  @VisibleForTesting
+  public KafkaRecordSupplier(
+  Map consumerProperties,
+  ObjectMapper sortingMapper,
+  Map consumerConfigs
+  )
+  {
+this.consumerProperties = consumerProperties;
+this.sortingMapper = sortingMapper;
+this.consumer = getKafkaConsumer(consumerConfigs);
+  }
 
 Review comment:
`getKafkaConsumer ` depends on another private method,  
`addConsumerPropertiesFromConfig`, move both methods to test file?


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] jnaous commented on a change in pull request #8825: Add note on JDBC libs for lookups

2019-11-05 Thread GitBox

jnaous commented on a change in pull request #8825: Add note on JDBC libs for 
lookups
URL: https://github.com/apache/incubator-druid/pull/8825#discussion_r342804983
 
 

 ##
 File path: docs/development/extensions-core/lookups-cached-global.md
 ##
 @@ -368,6 +368,12 @@ The JDBC lookups will poll a database to populate its 
local cache. If the `tsCol
 }
 ```
 
+> If using JDBC, you will need to add your database's client JAR files to the 
extension's directory.
+> For MySQL, you can get it from https://dev.mysql.com/downloads/connector/j/, 
and for Postgres, from
+> https://jdbc.postgresql.org/download.html or from the 
`extensions/postgresql-metadata-storage/`.
+> Copy or symlink the downloaded file to
+> `extensions/lookups-cached-global` under the distribution root directory.
 
 Review comment:
   Fixed


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] jnaous commented on a change in pull request #8825: Add note on JDBC libs for lookups

2019-11-05 Thread GitBox

jnaous commented on a change in pull request #8825: Add note on JDBC libs for 
lookups
URL: https://github.com/apache/incubator-druid/pull/8825#discussion_r342804962
 
 

 ##
 File path: docs/development/extensions-core/lookups-cached-global.md
 ##
 @@ -368,6 +368,12 @@ The JDBC lookups will poll a database to populate its 
local cache. If the `tsCol
 }
 ```
 
+> If using JDBC, you will need to add your database's client JAR files to the 
extension's directory.
+> For MySQL, you can get it from https://dev.mysql.com/downloads/connector/j/, 
and for Postgres, from
+> https://jdbc.postgresql.org/download.html or from the 
`extensions/postgresql-metadata-storage/`.
 
 Review comment:
   Fixed


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 commented on issue #8823: Add InputSource and InputFormat interfaces

2019-11-05 Thread GitBox

fjy commented on issue #8823: Add InputSource and InputFormat interfaces
URL: https://github.com/apache/incubator-druid/pull/8823#issuecomment-550100991
 
 
   One of the most exciting PRs on Druid ingestion in awhile. Glad we got it 
out.


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] vogievetsky opened a new pull request #8828: Web console: support new ingest spec format

2019-11-05 Thread GitBox

vogievetsky opened a new pull request #8828: Web console: support new ingest 
spec format
URL: https://github.com/apache/incubator-druid/pull/8828
 
 
   Makes the data loader work with the newer spec format defined by 
https://github.com/apache/incubator-druid/pull/8823
   
   
![image](https://user-images.githubusercontent.com/177816/68260263-52a11380-fff1-11e9-8cc1-84336fe1f89e.png)
   


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] lgtm-com[bot] commented on issue #8823: Add InputSource and InputFormat interfaces

2019-11-05 Thread GitBox

lgtm-com[bot] commented on issue #8823: Add InputSource and InputFormat 
interfaces
URL: https://github.com/apache/incubator-druid/pull/8823#issuecomment-550098205
 
 
   This pull request **fixes 1 alert** when merging 
d45158271d33b5795c56657e5f13f1553bcf1147 into 
3b602da8f7b0ff78071404b0cf6ca8a9831dd985 - [view on 
LGTM.com](https://lgtm.com/projects/g/apache/incubator-druid/rev/pr-1b97c0a722dde5c074bba1c9590d81227dd7eb0b)
   
   **fixed alerts:**
   
   * 1 for Dereferenced variable may be null


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] lgtm-com[bot] commented on issue #8810: Make submit task API similar to supervisor API

2019-11-05 Thread GitBox

lgtm-com[bot] commented on issue #8810: Make submit task API similar to 
supervisor API
URL: https://github.com/apache/incubator-druid/pull/8810#issuecomment-550098178
 
 
   This pull request **introduces 3 alerts** when merging 
c34c5cf8ed430b6dc3442c202287bb96e1f4cd3d into 
3b602da8f7b0ff78071404b0cf6ca8a9831dd985 - [view on 
LGTM.com](https://lgtm.com/projects/g/apache/incubator-druid/rev/pr-decab84744671084bcabea6e8f289110c5152705)
   
   **new alerts:**
   
   * 3 for Dereferenced variable may be null


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] jihoonson commented on issue #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

jihoonson commented on issue #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419#issuecomment-550073283
 
 
   The LGTM is failing with the same error even after I restarted once. Since 
TeamCity looks fine, I think it's an issue with LGTM (maybe it's caching 
something). I'll merge this PR shortly and will see other builds succeed.


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 (511fa74 -> 3b602da)

2019-11-05 Thread jihoonson

This is an automated email from the ASF dual-hosted git repository.

jihoonson pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git.


from 511fa74  Move maxFetchRetry to FetchConfig; rename OpenObject (#8776)
 add 3b602da  Bump Apache Thrift to 0.10.0 (#8419)

No new revisions were added by this update.

Summary of changes:
 extensions-contrib/thrift-extensions/pom.xml | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)


-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org

[GitHub] [incubator-druid] jihoonson merged pull request #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

jihoonson merged pull request #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419
 
 
   


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] vogievetsky commented on a change in pull request #8777: Web console: Interval input component

2019-11-05 Thread GitBox

vogievetsky commented on a change in pull request #8777: Web console: Interval 
input component
URL: https://github.com/apache/incubator-druid/pull/8777#discussion_r342884494
 
 

 ##
 File path: web-console/src/views/query-view/query-output/query-output.scss
 ##
 @@ -22,6 +22,7 @@
 top: 0;
 bottom: 0;
 width: 100%;
+font-variant-numeric: tabular-nums;
 
 Review comment:
   look at https://css-tricks.com/almanac/properties/f/font-variant-numeric/
   
   I think you also want to add `font-feature-settings: tnum`


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] stale[bot] commented on issue #6952: ServerSelector should always return the best segment

2019-11-05 Thread GitBox

stale[bot] commented on issue #6952: ServerSelector should always return the 
best segment 
URL: 
https://github.com/apache/incubator-druid/issues/6952#issuecomment-550076239
 
 
   This issue has been marked as stale due to 280 days of inactivity. It will 
be closed in 4 weeks if no further activity occurs. If this issue is still 
relevant, please simply write any comment. Even if closed, you can still revive 
the issue at any time or discuss it on the d...@druid.apache.org list. Thank 
you for your contributions.
   


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] vogievetsky commented on a change in pull request #8777: Web console: Interval input component

2019-11-05 Thread GitBox

vogievetsky commented on a change in pull request #8777: Web console: Interval 
input component
URL: https://github.com/apache/incubator-druid/pull/8777#discussion_r342883478
 
 

 ##
 File path: web-console/src/components/interval-input/interval-input.tsx
 ##
 @@ -0,0 +1,114 @@
+/*
+ * 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.
+ */
+
+import { Button, InputGroup, Popover, Position } from '@blueprintjs/core';
+import { DateRange, DateRangePicker } from '@blueprintjs/datetime';
+import { IconNames } from '@blueprintjs/icons';
+import React, { useState } from 'react';
+
+import './interval-input.scss';
+
+const CURRENT_YEAR = new Date().getUTCFullYear();
+
+export interface IntervalInputProps {
+  interval: string;
+  placeholder: string | undefined;
+  onValueChange: (interval: string) => void;
+}
+
+export const IntervalInput = React.memo(function IntervalInput(props: 
IntervalInputProps) {
+  const [tempInterval, setTempInterval] = useState();
+  const { interval, placeholder, onValueChange } = props;
+
+  function removeLocalTimezone(localDate: Date): Date {
+// Function removes the local timezone of the date and displays it in UTC
+return new Date(localDate.getTime() - localDate.getTimezoneOffset() * 
6);
+  }
+
+  function parseInterval(interval: string): DateRange {
+const dates = interval.split('/');
+if (dates.length !== 2) {
+  return [undefined, undefined];
+}
+const startDate = Date.parse(dates[0]) ? new Date(dates[0]) : undefined;
+const endDate = Date.parse(dates[1]) ? new Date(dates[1]) : undefined;
+// Must check if the start and end dates are within range
+return [
+  startDate && startDate.getFullYear() < CURRENT_YEAR - 20 ? undefined : 
startDate,
+  endDate && endDate.getFullYear() > CURRENT_YEAR ? undefined : endDate,
+];
+  }
+  function parseDateRange(localRange: DateRange): string {
+// This function takes in the dates selected from datepicker in local 
time, and displays them in UTC
+// Shall Blueprint make any changes to the way dates are selected, this 
function will have to be reworked
+const [localStartDate, localEndDate] = localRange;
+return `${
+  localStartDate
+? removeLocalTimezone(localStartDate)
+.toISOString()
+.substring(0, 19)
+: ''
+}/${
+  localEndDate
+? removeLocalTimezone(localEndDate)
+.toISOString()
+.substring(0, 19)
+: ''
+}`;
+  }
+
+  return (
+

[GitHub] [incubator-druid] vogievetsky commented on a change in pull request #8777: Web console: Interval input component

2019-11-05 Thread GitBox

vogievetsky commented on a change in pull request #8777: Web console: Interval 
input component
URL: https://github.com/apache/incubator-druid/pull/8777#discussion_r342883764
 
 

 ##
 File path: web-console/src/components/interval-input/interval-input.tsx
 ##
 @@ -0,0 +1,114 @@
+/*
+ * 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.
+ */
+
+import { Button, InputGroup, Popover, Position } from '@blueprintjs/core';
+import { DateRange, DateRangePicker } from '@blueprintjs/datetime';
+import { IconNames } from '@blueprintjs/icons';
+import React, { useState } from 'react';
+
+import './interval-input.scss';
+
+const CURRENT_YEAR = new Date().getUTCFullYear();
+
+export interface IntervalInputProps {
+  interval: string;
+  placeholder: string | undefined;
+  onValueChange: (interval: string) => void;
+}
+
+export const IntervalInput = React.memo(function IntervalInput(props: 
IntervalInputProps) {
+  const [tempInterval, setTempInterval] = useState();
+  const { interval, placeholder, onValueChange } = props;
+
+  function removeLocalTimezone(localDate: Date): Date {
+// Function removes the local timezone of the date and displays it in UTC
+return new Date(localDate.getTime() - localDate.getTimezoneOffset() * 
6);
+  }
+
+  function parseInterval(interval: string): DateRange {
+const dates = interval.split('/');
+if (dates.length !== 2) {
+  return [undefined, undefined];
+}
+const startDate = Date.parse(dates[0]) ? new Date(dates[0]) : undefined;
+const endDate = Date.parse(dates[1]) ? new Date(dates[1]) : undefined;
+// Must check if the start and end dates are within range
+return [
+  startDate && startDate.getFullYear() < CURRENT_YEAR - 20 ? undefined : 
startDate,
+  endDate && endDate.getFullYear() > CURRENT_YEAR ? undefined : endDate,
+];
+  }
+  function parseDateRange(localRange: DateRange): string {
+// This function takes in the dates selected from datepicker in local 
time, and displays them in UTC
+// Shall Blueprint make any changes to the way dates are selected, this 
function will have to be reworked
+const [localStartDate, localEndDate] = localRange;
+return `${
+  localStartDate
+? removeLocalTimezone(localStartDate)
+.toISOString()
+.substring(0, 19)
+: ''
+}/${
+  localEndDate
+? removeLocalTimezone(localEndDate)
+.toISOString()
+.substring(0, 19)
+: ''
+}`;
+  }
+
+  return (
+
+   {
+  onValueChange(parseDateRange(selectedRange));
+}}
+  />
+}
+position={Position.BOTTOM_RIGHT}
+  >
+
+  
+
+  }
+  onChange={(e: any) => {
+if (
+  (e.target.value.match(/^[\-0-9T:/]+$/g) && e.target.value.length <= 
39) ||
 
 Review comment:
   This is not what you want, you want to filter 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:
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] zhenxiao commented on issue #8809: Prohibit Futures.addCallback(Future, Callback)

2019-11-05 Thread GitBox

zhenxiao commented on issue #8809: Prohibit Futures.addCallback(Future, 
Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#issuecomment-550104154
 
 
   thank you, @leventov 
   comments addressed
   did a force push, put this work on top of 
https://github.com/apache/incubator-druid/pull/8700, so that compiles with 
forbidden-apis
   only the 2nd commit for review


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] lgtm-com[bot] commented on issue #8814: SQL: Add RAND() function.

2019-11-05 Thread GitBox

lgtm-com[bot] commented on issue #8814: SQL: Add RAND() function.
URL: https://github.com/apache/incubator-druid/pull/8814#issuecomment-550140092
 
 
   This pull request **introduces 2 alerts** when merging 
97a07809fe6fa92d566ef0ae4161a16d35a76e5a into 
3b602da8f7b0ff78071404b0cf6ca8a9831dd985 - [view on 
LGTM.com](https://lgtm.com/projects/g/apache/incubator-druid/rev/pr-1ed50d1b266829b1f20abea34b4a940bd29c4f1a)
   
   **new alerts:**
   
   * 2 for Missing format argument


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 #8819: Web console: show hollow circle when unavailable

2019-11-05 Thread GitBox

fjy merged pull request #8819: Web console: show hollow circle when unavailable
URL: https://github.com/apache/incubator-druid/pull/8819
 
 
   


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 (3b602da -> c2889ca)

2019-11-05 Thread fjy

This is an automated email from the ASF dual-hosted git repository.

fjy pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git.


from 3b602da  Bump Apache Thrift to 0.10.0 (#8419)
 add c2889ca  show hollow circle when unavailable (#8819)

No new revisions were added by this update.

Summary of changes:
 web-console/src/views/datasource-view/datasource-view.tsx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org

[incubator-druid] branch master updated (c2889ca -> 6f7fbeb)

2019-11-05 Thread fjy

This is an automated email from the ASF dual-hosted git repository.

fjy pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git.


from c2889ca  show hollow circle when unavailable (#8819)
 add 6f7fbeb  Fix logo overflow (#8817)

No new revisions were added by this update.

Summary of changes:
 web-console/src/components/header-bar/header-bar.scss | 1 +
 1 file changed, 1 insertion(+)


-
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 #8817: Web console: Fix logo overflow

2019-11-05 Thread GitBox

fjy merged pull request #8817: Web console: Fix logo overflow
URL: https://github.com/apache/incubator-druid/pull/8817
 
 
   


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 fjy-patch-1 created (now 48683d7)

2019-11-05 Thread fjy

This is an automated email from the ASF dual-hosted git repository.

fjy pushed a change to branch fjy-patch-1
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git.


  at 48683d7  Update README.md

No new revisions were added by this update.


-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org

[GitHub] [incubator-druid] fjy opened a new pull request #8829: Update README.md

2019-11-05 Thread GitBox

fjy opened a new pull request #8829: Update README.md
URL: https://github.com/apache/incubator-druid/pull/8829
 
 
   small edits to the druid readme
   
   
   
   Fixes #.
   
   
   
   
   
   ### Description
   
   
   
   
   
   
   
    Fixed the bug ...
    Renamed the class ...
    Added a forbidden-apis entry ...
   
   
   
   
   
   
   
   
   
   
   
   
   This PR has:
   - [ ] been self-reviewed.
  - [ ] using the [concurrency 
checklist](https://github.com/apache/incubator-druid/blob/master/dev/code-review/concurrency.md)
 (Remove this item if the PR doesn't have any relation to concurrency.)
   - [ ] added documentation for new or modified features or behaviors.
   - [ ] added Javadocs for most classes and all non-trivial methods. Linked 
related entities via Javadoc links.
   - [ ] added or updated version, license, or notice information in 
[licenses.yaml](https://github.com/apache/incubator-druid/blob/master/licenses.yaml)
   - [ ] added comments explaining the "why" and the intent of the code 
wherever would not be obvious for an unfamiliar reader.
   - [ ] added unit tests or modified existing tests to cover new code paths.
   - [ ] added integration tests.
   - [ ] been tested in a test Druid cluster.
   
   
   
   
   
   # Key changed/added classes in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
   


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 #8808: use copy-on-write list in InMemoryAppender

2019-11-05 Thread GitBox

leventov commented on issue #8808: use copy-on-write list in InMemoryAppender
URL: https://github.com/apache/incubator-druid/pull/8808#issuecomment-550170383
 
 
   @LoriLori `logEvents.toArray()` doesn't solve the race condition - it may 
still race with an addition to `logEvents` and throw some exception, unless you 
make both `append()` and `getLogEvents()` synchronized.
   
   Since this class resides in tests, not production, the performance doesn't 
matter and we shouldn't waste time comparing `synchronized` and 
`CopyOnWriteArrayList`. While `CopyOnWriteArrayList` solution is a bit simpler.


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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342951053
 
 

 ##
 File path: 
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexPhaseRunner.java
 ##
 @@ -250,7 +254,8 @@ public void onFailure(Throwable t)
 LOG.error(t, "Error while running a task for subTaskSpec[%s]", 
spec);
 taskCompleteEvents.offer(SubTaskCompleteEvent.fail(spec, t));
   }
-}
+},
+blockingQueueHandler
 
 Review comment:
   Correction - `BlockingQueue.offer()` is still needs to enter a critical 
section, but it may be considered "mostly non-blocking", so directExecutor() 
should still be OK


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] vogievetsky commented on issue #8805: Web console: fine grained capabilities / graceful degradation

2019-11-05 Thread GitBox

vogievetsky commented on issue #8805: Web console: fine grained capabilities / 
graceful degradation
URL: https://github.com/apache/incubator-druid/pull/8805#issuecomment-550185623
 
 
   One step closer to getting rid of the old consoles!


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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342951053
 
 

 ##
 File path: 
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexPhaseRunner.java
 ##
 @@ -250,7 +254,8 @@ public void onFailure(Throwable t)
 LOG.error(t, "Error while running a task for subTaskSpec[%s]", 
spec);
 taskCompleteEvents.offer(SubTaskCompleteEvent.fail(spec, t));
   }
-}
+},
+blockingQueueHandler
 
 Review comment:
   Correction - `BlockingQueue.offer()` still needs to enter a critical 
section, but it may be considered "mostly non-blocking", so directExecutor() 
should still be OK


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] vogievetsky merged pull request #8805: Web console: fine grained capabilities / graceful degradation

2019-11-05 Thread GitBox

vogievetsky merged pull request #8805: Web console: fine grained capabilities / 
graceful degradation
URL: https://github.com/apache/incubator-druid/pull/8805
 
 
   


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 (6f7fbeb -> 7addfc2)

2019-11-05 Thread vogievetsky

This is an automated email from the ASF dual-hosted git repository.

vogievetsky pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git.


from 6f7fbeb  Fix logo overflow (#8817)
 add 7addfc2  Web console: fine grained capabilities / graceful degradation 
(#8805)

No new revisions were added by this update.

Summary of changes:
 .../__snapshots__/header-bar.spec.tsx.snap |  21 +-
 .../src/components/header-bar/header-bar.scss  |   2 +-
 .../src/components/header-bar/header-bar.spec.tsx  |   4 +-
 .../src/components/header-bar/header-bar.tsx   |  51 ++--
 web-console/src/console-application.tsx| 147 
 .../coordinator-dynamic-config-dialog.tsx  |  18 +-
 .../src/dialogs/doctor-dialog/doctor-checks.tsx|  40 ++--
 web-console/src/utils/capabilities.ts  | 140 ++-
 web-console/src/utils/local-storage-keys.tsx   |   4 +-
 .../views/datasource-view/datasource-view.spec.tsx |   4 +-
 .../src/views/datasource-view/datasource-view.tsx  |  27 +--
 .../__snapshots__/home-view.spec.tsx.snap  |  42 +++-
 .../datasources-card/datasources-card.spec.tsx |   4 +-
 .../datasources-card/datasources-card.tsx  |   7 +-
 web-console/src/views/home-view/home-view.scss |   2 +-
 web-console/src/views/home-view/home-view.spec.tsx |   4 +-
 web-console/src/views/home-view/home-view.tsx  |   4 +-
 .../home-view/segments-card/segments-card.spec.tsx |   4 +-
 .../home-view/segments-card/segments-card.tsx  |  18 +-
 .../home-view/servers-card/servers-card.spec.tsx   |  31 ---
 .../__snapshots__/services-card.spec.tsx.snap} |   8 +-
 .../services-card/services-card.spec.tsx}  |  14 +-
 .../services-card.tsx} |  82 ---
 .../supervisors-card/supervisors-card.spec.tsx |   4 +-
 .../supervisors-card/supervisors-card.tsx  |   6 +-
 .../views/home-view/tasks-card/tasks-card.spec.tsx |   4 +-
 .../src/views/home-view/tasks-card/tasks-card.tsx  |  20 +-
 web-console/src/views/index.ts |   2 +-
 .../src/views/load-data-view/load-data-view.tsx|  11 +-
 .../src/views/segments-view/segments-view.spec.tsx |   3 +-
 .../src/views/segments-view/segments-view.tsx  |  61 ++---
 .../__snapshots__/services-view.spec.tsx.snap} |  16 +-
 .../services-view.scss}|   0
 .../services-view.spec.tsx}|  16 +-
 .../services-view.tsx} | 259 +++--
 .../src/views/task-view/tasks-view.spec.tsx|   4 +-
 web-console/src/views/task-view/tasks-view.tsx |  16 +-
 37 files changed, 624 insertions(+), 476 deletions(-)
 delete mode 100644 
web-console/src/views/home-view/servers-card/servers-card.spec.tsx
 rename 
web-console/src/views/home-view/{servers-card/__snapshots__/servers-card.spec.tsx.snap
 => services-card/__snapshots__/services-card.spec.tsx.snap} (87%)
 copy web-console/src/{components/center-message/center-message.spec.tsx => 
views/home-view/services-card/services-card.spec.tsx} (78%)
 rename web-console/src/views/home-view/{servers-card/servers-card.tsx => 
services-card/services-card.tsx} (61%)
 rename 
web-console/src/views/{servers-view/__snapshots__/servers-view.spec.tsx.snap => 
services-view/__snapshots__/services-view.spec.tsx.snap} (96%)
 rename web-console/src/views/{servers-view/servers-view.scss => 
services-view/services-view.scss} (100%)
 rename web-console/src/views/{servers-view/servers-view.spec.tsx => 
services-view/services-view.spec.tsx} (75%)
 rename web-console/src/views/{servers-view/servers-view.tsx => 
services-view/services-view.tsx} (72%)


-
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 #8564: Fix ambiguity about IndexerSQLMetadataStorageCoordinator.getUsedSegmentsForInterval() returning only non-overshadowed o

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8564: Fix ambiguity about 
IndexerSQLMetadataStorageCoordinator.getUsedSegmentsForInterval() returning 
only non-overshadowed or all used segments
URL: https://github.com/apache/incubator-druid/pull/8564#discussion_r342938668
 
 

 ##
 File path: 
server/src/main/java/org/apache/druid/indexing/overlord/Segments.java
 ##
 @@ -0,0 +1,50 @@
+/*
+ * 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.druid.indexing.overlord;
+
+/**
+ * This enum is used as a parameter for several methods in {@link 
IndexerMetadataStorageCoordinator}, specifying whether
+ * only visible segments, or visible as well as overshadowed segments should 
be included in results.
+ *
+ * Visibility (and overshadowness - these terms are antonyms) may be defined 
on an interval (or a series of intervals).
+ * Consider the following example:
+ *
+ * || I
+ *   || S'
+ *   |---| S
+ *
+ * Here, I denotes an interval in question, S and S' are segments. S' is newer 
(has a higher version) than S.
+ * Segment S is overshadowed (by S') on the interval I, though it's visible 
(non-overshadowed) outside of I: more
+ * specifically, it's visible on the interval [end of S', end of S].
+ *
+ * A segment is considered visible on a series of intervals if it's visible on 
any of the intervals in the series. A
+ * segment is considered (fully) overshadowed on a series of intervals if it's 
overshadowed (= non-visible) on all of
+ * the intervals in the series.
+ *
+ * If not specified otherwise, visibility (or overshadowness) should be 
assumed on the interval (-inf, +inf). This
+ * visibility may also be called "global" or "general" visibility.
 
 Review comment:
   Ok, removed the last sentence.


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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342947361
 
 

 ##
 File path: 
indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java
 ##
 @@ -251,7 +251,9 @@ public void onFailure(Throwable t)
   {
 submitNoticeToExec(new StatusNotice(task, 
TaskStatus.failure(task.getId(;
   }
-}
+},
+// The callback is non-blocking and quick, so it's OK to schedule it 
using directExecutor()
 
 Review comment:
   Please add a javadoc comment to `submitNoticeToExec()` like "This method is 
mostly non-blocking because exec is a ThreadPoolExecutor with unbounded queue 
used by default."
   
   Correspondingly, this comment also should say "mostly non-blocking", not 
just "non-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:
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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342940087
 
 

 ##
 File path: 
core/src/main/java/org/apache/druid/java/util/common/concurrent/ListenableFutures.java
 ##
 @@ -61,15 +61,19 @@ public void onFailure(Throwable t)
   {
 finalFuture.setException(t);
   }
-});
+},
+// The callback is non-blocking and quick, so it's OK to schedule 
it using directExecutor()
 
 Review comment:
   Too much indentation


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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342940115
 
 

 ##
 File path: 
core/src/main/java/org/apache/druid/java/util/common/concurrent/ListenableFutures.java
 ##
 @@ -61,15 +61,19 @@ public void onFailure(Throwable t)
   {
 finalFuture.setException(t);
   }
-});
+},
+// The callback is non-blocking and quick, so it's OK to schedule 
it using directExecutor()
+Execs.directExecutor());
   }
 
   @Override
   public void onFailure(Throwable t)
   {
 finalFuture.setException(t);
   }
-});
+},
+// The callback is non-blocking and quick, so it's OK to schedule it 
using directExecutor()
 
 Review comment:
   Same


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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342945408
 
 

 ##
 File path: 
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java
 ##
 @@ -556,7 +559,8 @@ private void handleStatus(final TaskStatus status)
  .emit();
 }
   }
-}
+},
+statusHandler
 
 Review comment:
   It's non-obvious from this point in code to see that `attachCallbacks()` is 
called from a different thread pool than `statusFuture` is completed.
   
   There are options to improve:
- Propagate `Executor` argument and pass `statusHandler` only in 
`manage()`, and, additionally, amend the javadoc comment for `manage()` noting 
that it is run from `{@link #managerExec}`.
- Just add a more elaborate comment right here.
   
   In either case, it will be non-obvious for readers why different executors 
demand a special executor. So the comment should look something like that:
   "Using dedicated statusHandler executor instead of directExecutor() because 
the callback's onSuccess() is not trivial and because the statusFuture is 
completed in some incapsulated thread pool in TaskRunner; directExecutor() may 
create operational instability and subtle dependency between components here. 
See 
https://github.com/code-review-checklists/java-concurrency#cf-beware-non-async 
for details."
   
   Something similar should be done in all other places where you use a custom 
executor instead of directExecutor().


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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342948998
 
 

 ##
 File path: 
server/src/main/java/org/apache/druid/server/http/SegmentListerResource.java
 ##
 @@ -205,7 +209,8 @@ public void onFailure(Throwable th)
   log.debug(ex, "Request timed out or closed already.");
 }
   }
-}
+},
 
 Review comment:
   Same. This code looks similar to `TaskManagementResource`. Maybe the common 
logic should be extracted. Or, there should be comments to both of them noting 
that these classes are written in a similar way so changes, fixes, refactorings 
should be applied to both classes at the same time.


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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342939947
 
 

 ##
 File path: 
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexPhaseRunner.java
 ##
 @@ -250,7 +254,8 @@ public void onFailure(Throwable t)
 LOG.error(t, "Error while running a task for subTaskSpec[%s]", 
spec);
 taskCompleteEvents.offer(SubTaskCompleteEvent.fail(spec, t));
   }
-}
+},
+blockingQueueHandler
 
 Review comment:
   `taskCompleteEvents.offer(completeEvent);` -- `BlockingQueue.offer()` is 
actually non-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:
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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342948270
 
 

 ##
 File path: 
indexing-service/src/main/java/org/apache/druid/indexing/worker/http/TaskManagementResource.java
 ##
 @@ -192,7 +196,8 @@ public void onFailure(Throwable th)
   log.debug(ex, "Request timed out or closed already.");
 }
   }
-}
+},
 
 Review comment:
   Some weird asynchrony going on in this method (at least it looks so), so it 
demands more comments


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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342945993
 
 

 ##
 File path: 
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java
 ##
 @@ -1008,7 +1014,8 @@ public void onFailure(Throwable t)
 log.error(t, "Error while publishing segments for 
sequenceNumber[%s]", sequenceMetadata);
 handoffFuture.setException(t);
   }
-}
+},
+sequencePersistExecutor
 
 Review comment:
   I withdraw from analyzing execution flows and thread pool relationships 
here. @jihoonson could you please vet this?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
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)

2019-11-05 Thread GitBox

leventov commented on a change in pull request #8809: Prohibit 
Futures.addCallback(Future, Callback)
URL: https://github.com/apache/incubator-druid/pull/8809#discussion_r342940649
 
 

 ##
 File path: 
indexing-service/src/main/java/org/apache/druid/indexing/overlord/RemoteTaskRunner.java
 ##
 @@ -256,7 +256,9 @@ public void onFailure(Throwable throwable)
   waitingForMonitor.notifyAll();
 }
   }
-}
+},
+// The callback is non-blocking and quick, so it's OK to 
schedule it using directExecutor()
 
 Review comment:
   `synchronized (waitingForMonitor)` **is blocking**. You may argue that this 
lock is (almost) free, for some reasons, so then the callback should still be 
considered "mostly non-blocking" and OK to schedule on directExecutor(), but in 
any case it demands a more elaborate comment


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] clintropolis commented on a change in pull request #8805: Web console: fine grained capabilities / graceful degradation

2019-11-05 Thread GitBox

clintropolis commented on a change in pull request #8805: Web console: fine 
grained capabilities / graceful degradation
URL: https://github.com/apache/incubator-druid/pull/8805#discussion_r342949872
 
 

 ##
 File path: 
web-console/src/views/home-view/datasources-card/datasources-card.tsx
 ##
 @@ -50,14 +50,17 @@ export class DatasourcesCard extends React.PureComponent<
 this.datasourceQueryManager = new QueryManager({
   processQuery: async capabilities => {
 let datasources: string[];
-if (capabilities !== 'no-sql') {
+if (capabilities.hasSql()) {
   datasources = await queryDruidSql({
 query: `SELECT datasource FROM sys.segments GROUP BY 1`,
   });
-} else {
+} else if (capabilities.hasCoordinatorAccess()) {
   const datasourcesResp = await 
axios.get('/druid/coordinator/v1/datasources');
   datasources = datasourcesResp.data;
+} else {
+  throw new Error(`must have SQL or coordinator access`);
 
 Review comment:
   I see a handful of these similar themed errors, would it make sense to make 
some custom errors like `NoCoordinatorError`, `NoOverlordError`, etc so you 
don't have to repeat the string message?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
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 closed issue #8824: teamcity build is failing with node module related issue

2019-11-05 Thread GitBox

leventov closed issue #8824: teamcity build is failing with node module related 
issue
URL: https://github.com/apache/incubator-druid/issues/8824
 
 
   


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] himanshug commented on issue #8802: add documentation for druid docker and k8s operator

2019-11-05 Thread GitBox

himanshug commented on issue #8802: add documentation for druid docker and k8s 
operator
URL: https://github.com/apache/incubator-druid/pull/8802#issuecomment-549914815
 
 
   TeamCity build appears to have failed for a totally unrelated reason
   ```
   [16:44:48]org.apache.druid:druid-console (3m:01s)
   [16:46:24][org.apache.druid:druid-console] [BABEL] Note: The code generator 
has deoptimised the styling of 
/mnt/agent/work/9f653ecb5b5406e8/web-console/node_modules/brace/index.js as it 
exceeds the max of 500KB.
   [17:01:17]Process exited with code 1
   ```
   retrying 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:
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] himanshug commented on issue #8578: parallel broker merges on fork join pool

2019-11-05 Thread GitBox

himanshug commented on issue #8578: parallel broker merges on fork join pool
URL: https://github.com/apache/incubator-druid/pull/8578#issuecomment-549904438
 
 
   @clintropolis thanks for all the benchmarks, I haven't had the opportunity 
to look at the new developments yet but get back to reviewing this week.
   
   one thing, I am not sure if taken care or not, many people run the druid 
processes inside docker containers where 
Runtime.getRuntime().availableProcessors() returns the available processes from 
host and not from the "container". ( 
https://bugs.openjdk.java.net/browse/JDK-8140793 , I think it has been changed 
in jdk10) .  Given the sensitivity of performance to availableProcessors() 
returned value, it might be good to make that area a bit configurable if not 
already. I will hopefully offer more specific suggestion when reviewing again.


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] himanshug commented on issue #8802: add documentation for druid docker and k8s operator

2019-11-05 Thread GitBox

himanshug commented on issue #8802: add documentation for druid docker and k8s 
operator
URL: https://github.com/apache/incubator-druid/pull/8802#issuecomment-549901339
 
 
   thanks @nishantmonu51 and @clintropolis , I will have the spelling build 
fixed and then merge once that is successful.


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] clintropolis commented on issue #8578: parallel broker merges on fork join pool

2019-11-05 Thread GitBox

clintropolis commented on issue #8578: parallel broker merges on fork join pool
URL: https://github.com/apache/incubator-druid/pull/8578#issuecomment-549966959
 
 
   >@clintropolis thanks for all the benchmarks, I haven't had the opportunity 
to look at the new developments yet but get back to reviewing this week.
   
   Yeah, no problem, thanks for asking the hard questions to make me collect 
them, the result is the PR is in a better state than before them :metal:. The 
production part of the code hasn't really changed much in the last couple of 
weeks other than a few lines to change behavior of the parallelism computing 
method, and mostly changes to the default values.
   
   >Given the sensitivity of performance to availableProcessors() returned 
value, it might be good to make that area a bit configurable if not already. I 
will hopefully offer more specific suggestion when reviewing again.
   
   This stuff should all be controllable via configs, 
`druid.processing.merge.pool.parallelism` to control the FJP pool size, and 
`druid.merge.pool.defaultMaxQueryParallelism` to control individual query max 
parallelism (this one can also be set on the query context through 
`parallelMergeParallelism`).
   
   


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] clintropolis edited a comment on issue #8578: parallel broker merges on fork join pool

2019-11-05 Thread GitBox

clintropolis edited a comment on issue #8578: parallel broker merges on fork 
join pool
URL: https://github.com/apache/incubator-druid/pull/8578#issuecomment-549966959
 
 
   >@clintropolis thanks for all the benchmarks, I haven't had the opportunity 
to look at the new developments yet but get back to reviewing this week.
   
   Yeah, no problem, thanks for asking the hard questions to make me collect 
them, the result is the PR is in a better state than before them :metal:. The 
production part of the code hasn't really changed much in the last couple of 
weeks other than a few lines to change behavior of the parallelism computing 
method, and mostly changes to the default values.
   
   >Given the sensitivity of performance to availableProcessors() returned 
value, it might be good to make that area a bit configurable if not already. I 
will hopefully offer more specific suggestion when reviewing again.
   
   This stuff should all be controllable via configs, 
`druid.processing.merge.pool.parallelism` to control the FJP pool size, and 
`druid.merge.pool.defaultMaxQueryParallelism` to control individual query max 
parallelism (this one can also be set on the query context through 
`parallelMergeParallelism`).
   
   In a follow-up I think I will also try to add additional information to the 
cluster tuning guide docs, since I have a pretty good idea how this 
implementation performs now which I think we can use to advise operators.
   


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] ccaominh commented on a change in pull request #8748: Use earliest offset on kafka newly discovered partitions

2019-11-05 Thread GitBox

ccaominh commented on a change in pull request #8748: Use earliest offset on 
kafka newly discovered partitions
URL: https://github.com/apache/incubator-druid/pull/8748#discussion_r342768107
 
 

 ##
 File path: 
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
 ##
 @@ -259,4 +260,36 @@ private static void wrapExceptions(Runnable runnable)
   return null;
 });
   }
+
+  @VisibleForTesting
+  private KafkaConsumer getKafkaConsumer(Map 
consumerConfigs)
+  {
+final Properties props = new Properties();
+addConsumerPropertiesFromConfig(props, sortingMapper, consumerProperties);
+props.putAll(consumerConfigs);
+
+ClassLoader currCtxCl = Thread.currentThread().getContextClassLoader();
+try {
+  
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
+  Deserializer keyDeserializerObject = getKafkaDeserializer(props, 
"key.deserializer");
+  Deserializer valueDeserializerObject = getKafkaDeserializer(props, 
"value.deserializer");
+
+  return new KafkaConsumer<>(props, keyDeserializerObject, 
valueDeserializerObject);
+}
+finally {
+  Thread.currentThread().setContextClassLoader(currCtxCl);
+}
+  }
+
+  @VisibleForTesting
+  public KafkaRecordSupplier(
+  Map consumerProperties,
+  ObjectMapper sortingMapper,
+  Map consumerConfigs
+  )
+  {
+this.consumerProperties = consumerProperties;
+this.sortingMapper = sortingMapper;
+this.consumer = getKafkaConsumer(consumerConfigs);
+  }
 
 Review comment:
   Instead of adding a constructor that adds a `Map 
consumerConfigs` parameter, how about making it:
   ```
   @VisibleForTesting
   public KafkaRecordSupplier(
   Map consumerProperties,
   ObjectMapper sortingMapper,
   KafkaConsumer consumer
   )
   ```
   
   You'd move this new constructor to the top of this file below the original 
one and then have the original one delegate to your new one by passing in the 
existing `getKafkaConsumer()`. In `KafkaSupervisorTest`, for your regression 
test, you'd then use your new `@VisibleForTesting` constructor and move the 
method you added for creating a `KafkaConsumer` there since it's only intended 
for 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.
 
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] clintropolis commented on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

clintropolis commented on issue #8822: optimize numeric column null value 
checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-549997171
 
 
   >It looks like the operation in question 
(VectorSelectorUtils.populateNullVector) is actually simulating the extraction 
of a mask from the bitmap at the offset of the current vector. Perhaps, with 
the ability to skip to the next non empty vector (given a vector width), it 
would be quite easy to implement this as an iterator which returns masks on 
each call to next. 
   
   That sounds nice. I was planning on digging into this for future work to see 
if I could improve the null vector construction, but even just using the 
iterator should be an improvement over the existing code, so I didn't 
investigate that as part of this PR.


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] SEKIRO-J commented on a change in pull request #8748: Use earliest offset on kafka newly discovered partitions

2019-11-05 Thread GitBox

SEKIRO-J commented on a change in pull request #8748: Use earliest offset on 
kafka newly discovered partitions
URL: https://github.com/apache/incubator-druid/pull/8748#discussion_r342773057
 
 

 ##
 File path: 
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
 ##
 @@ -259,4 +260,36 @@ private static void wrapExceptions(Runnable runnable)
   return null;
 });
   }
+
+  @VisibleForTesting
+  private KafkaConsumer getKafkaConsumer(Map 
consumerConfigs)
+  {
+final Properties props = new Properties();
+addConsumerPropertiesFromConfig(props, sortingMapper, consumerProperties);
+props.putAll(consumerConfigs);
+
+ClassLoader currCtxCl = Thread.currentThread().getContextClassLoader();
+try {
+  
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
+  Deserializer keyDeserializerObject = getKafkaDeserializer(props, 
"key.deserializer");
+  Deserializer valueDeserializerObject = getKafkaDeserializer(props, 
"value.deserializer");
+
+  return new KafkaConsumer<>(props, keyDeserializerObject, 
valueDeserializerObject);
+}
+finally {
+  Thread.currentThread().setContextClassLoader(currCtxCl);
+}
+  }
+
+  @VisibleForTesting
+  public KafkaRecordSupplier(
+  Map consumerProperties,
+  ObjectMapper sortingMapper,
+  Map consumerConfigs
+  )
+  {
+this.consumerProperties = consumerProperties;
+this.sortingMapper = sortingMapper;
+this.consumer = getKafkaConsumer(consumerConfigs);
+  }
 
 Review comment:
   `getKafkaConsumer()` is a private 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.
 
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] SEKIRO-J commented on a change in pull request #8748: Use earliest offset on kafka newly discovered partitions

2019-11-05 Thread GitBox

SEKIRO-J commented on a change in pull request #8748: Use earliest offset on 
kafka newly discovered partitions
URL: https://github.com/apache/incubator-druid/pull/8748#discussion_r342773057
 
 

 ##
 File path: 
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
 ##
 @@ -259,4 +260,36 @@ private static void wrapExceptions(Runnable runnable)
   return null;
 });
   }
+
+  @VisibleForTesting
+  private KafkaConsumer getKafkaConsumer(Map 
consumerConfigs)
+  {
+final Properties props = new Properties();
+addConsumerPropertiesFromConfig(props, sortingMapper, consumerProperties);
+props.putAll(consumerConfigs);
+
+ClassLoader currCtxCl = Thread.currentThread().getContextClassLoader();
+try {
+  
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
+  Deserializer keyDeserializerObject = getKafkaDeserializer(props, 
"key.deserializer");
+  Deserializer valueDeserializerObject = getKafkaDeserializer(props, 
"value.deserializer");
+
+  return new KafkaConsumer<>(props, keyDeserializerObject, 
valueDeserializerObject);
+}
+finally {
+  Thread.currentThread().setContextClassLoader(currCtxCl);
+}
+  }
+
+  @VisibleForTesting
+  public KafkaRecordSupplier(
+  Map consumerProperties,
+  ObjectMapper sortingMapper,
+  Map consumerConfigs
+  )
+  {
+this.consumerProperties = consumerProperties;
+this.sortingMapper = sortingMapper;
+this.consumer = getKafkaConsumer(consumerConfigs);
+  }
 
 Review comment:
   getKafkaConsumer() is a private 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.
 
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] capistrant opened a new issue #8826: druid-basic-security - allow LDAP groupMapping updates

2019-11-05 Thread GitBox

capistrant opened a new issue #8826: druid-basic-security - allow LDAP 
groupMapping updates
URL: https://github.com/apache/incubator-druid/issues/8826
 
 
   ### Description
   
   An LDAP group mapping in druid-basic-security can only be created or deleted 
in its current implementation. This presents a problem when I want to do 
something like add a role to a mapping. I believe what is needed is an 
enhancement to allow `PUT` method. This method is idempotent and will create a 
new group mapping or overwrite an existing group mapping if it already exists. 
The result always being a group mapping with the attributes listed in the JSON 
body of the `PUT`.
   
   The role permissions endpoint has similar functionality in which a `POST` 
request to the permissions endpoint will overwrite any existing permissions 
list with what is in the JSON body. Instead of using `PUT` for this 
enhancement, we could mimic that functionality. 
   
   ### Motivation
   
   * Users may want to update groupMappings. They are not going to want to 
delete and re-create, which is the only mechanism available today according to 
the docs. Instead they will want to update in one HTTP call.
   


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] SEKIRO-J commented on a change in pull request #8748: Use earliest offset on kafka newly discovered partitions

2019-11-05 Thread GitBox

SEKIRO-J commented on a change in pull request #8748: Use earliest offset on 
kafka newly discovered partitions
URL: https://github.com/apache/incubator-druid/pull/8748#discussion_r342776259
 
 

 ##
 File path: 
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
 ##
 @@ -259,4 +260,36 @@ private static void wrapExceptions(Runnable runnable)
   return null;
 });
   }
+
+  @VisibleForTesting
+  private KafkaConsumer getKafkaConsumer(Map 
consumerConfigs)
+  {
+final Properties props = new Properties();
+addConsumerPropertiesFromConfig(props, sortingMapper, consumerProperties);
+props.putAll(consumerConfigs);
+
+ClassLoader currCtxCl = Thread.currentThread().getContextClassLoader();
+try {
+  
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
+  Deserializer keyDeserializerObject = getKafkaDeserializer(props, 
"key.deserializer");
+  Deserializer valueDeserializerObject = getKafkaDeserializer(props, 
"value.deserializer");
+
+  return new KafkaConsumer<>(props, keyDeserializerObject, 
valueDeserializerObject);
+}
+finally {
+  Thread.currentThread().setContextClassLoader(currCtxCl);
+}
+  }
+
+  @VisibleForTesting
+  public KafkaRecordSupplier(
+  Map consumerProperties,
+  ObjectMapper sortingMapper,
+  Map consumerConfigs
+  )
+  {
+this.consumerProperties = consumerProperties;
+this.sortingMapper = sortingMapper;
+this.consumer = getKafkaConsumer(consumerConfigs);
+  }
 
 Review comment:
   and `getKafkaConsumer ` depends on another private method,  
`addConsumerPropertiesFromConfig`, copy both method to test file?


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] clintropolis commented on issue #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

clintropolis commented on issue #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419#issuecomment-550024263
 
 
   >though maybe it should be since there is a NOTICE file, 
https://github.com/apache/thrift/blob/master/NOTICE.
   
   Responding to myself, but I don't think we need to worry about updating 
`licenses.yaml` in this PR.


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] clintropolis commented on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

clintropolis commented on issue #8822: optimize numeric column null value 
checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-550030106
 
 
   >For example, if we're doing a longSum operation on a
   column, what would the cost per row be, and what would the percentage
   overhead of this check be?
   
   The benchmark added in this PR can't determine that, all I can provide is 
the fixed cost per row of having nulls. That seems useful to know, but also 
time intensive so I might not get to it as part of this PR.


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] lemire commented on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

lemire commented on issue #8822: optimize numeric column null value checking 
for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-550029911
 
 
   The plots are great indeed. Bravo!


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] jihoonson commented on a change in pull request #8564: Fix ambiguity about IndexerSQLMetadataStorageCoordinator.getUsedSegmentsForInterval() returning only non-overshadowed

2019-11-05 Thread GitBox

jihoonson commented on a change in pull request #8564: Fix ambiguity about 
IndexerSQLMetadataStorageCoordinator.getUsedSegmentsForInterval() returning 
only non-overshadowed or all used segments
URL: https://github.com/apache/incubator-druid/pull/8564#discussion_r342834195
 
 

 ##
 File path: 
server/src/main/java/org/apache/druid/indexing/overlord/Segments.java
 ##
 @@ -0,0 +1,50 @@
+/*
+ * 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.druid.indexing.overlord;
+
+/**
+ * This enum is used as a parameter for several methods in {@link 
IndexerMetadataStorageCoordinator}, specifying whether
+ * only visible segments, or visible as well as overshadowed segments should 
be included in results.
+ *
+ * Visibility (and overshadowness - these terms are antonyms) may be defined 
on an interval (or a series of intervals).
+ * Consider the following example:
+ *
+ * || I
+ *   || S'
+ *   |---| S
+ *
+ * Here, I denotes an interval in question, S and S' are segments. S' is newer 
(has a higher version) than S.
+ * Segment S is overshadowed (by S') on the interval I, though it's visible 
(non-overshadowed) outside of I: more
+ * specifically, it's visible on the interval [end of S', end of S].
+ *
+ * A segment is considered visible on a series of intervals if it's visible on 
any of the intervals in the series. A
+ * segment is considered (fully) overshadowed on a series of intervals if it's 
overshadowed (= non-visible) on all of
+ * the intervals in the series.
+ *
+ * If not specified otherwise, visibility (or overshadowness) should be 
assumed on the interval (-inf, +inf). This
+ * visibility may also be called "global" or "general" visibility.
 
 Review comment:
   This new doc looks nice. Just one question is, do we need the concept of 
"global visibility"? Seems like it's enough to say `If not specified otherwise, 
visibility (or overshadowness) should be assumed on the interval (-inf, +inf).`.


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] himanshug opened a new issue #8824: teamcity build is failing with node module related issue

2019-11-05 Thread GitBox

himanshug opened a new issue #8824: teamcity build is failing with node module 
related issue
URL: https://github.com/apache/incubator-druid/issues/8824
 
 
   Message Printed..
   ```
   [18:05:56]org.apache.druid:druid-console (3m:39s)
   [18:08:39][org.apache.druid:druid-console] [BABEL] Note: The code generator 
has deoptimised the styling of 
/mnt/agent/work/9f653ecb5b5406e8/web-console/node_modules/brace/index.js as it 
exceeds the max of 500KB.
   [18:20:09]Process exited with code 1
   ```
   
   Ref: 
https://teamcity.jetbrains.com/viewLog.html?buildId=2618987=OpenSourceProjects_Druid_InspectionsPullRequests
   
   @vogievetsky do you know what could be wrong ?


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] ccaominh commented on a change in pull request #8825: Add note on JDBC libs for lookups

2019-11-05 Thread GitBox

ccaominh commented on a change in pull request #8825: Add note on JDBC libs for 
lookups
URL: https://github.com/apache/incubator-druid/pull/8825#discussion_r342775228
 
 

 ##
 File path: docs/development/extensions-core/lookups-cached-global.md
 ##
 @@ -368,6 +368,12 @@ The JDBC lookups will poll a database to populate its 
local cache. If the `tsCol
 }
 ```
 
+> If using JDBC, you will need to add your database's client JAR files to the 
extension's directory.
+> For MySQL, you can get it from https://dev.mysql.com/downloads/connector/j/, 
and for Postgres, from
+> https://jdbc.postgresql.org/download.html or from the 
`extensions/postgresql-metadata-storage/`.
+> Copy or symlink the downloaded file to
+> `extensions/lookups-cached-global` under the distribution root directory.
 
 Review comment:
   I believe the directory should be `extensions/druid-lookups-cached-global`.


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] ccaominh commented on a change in pull request #8825: Add note on JDBC libs for lookups

2019-11-05 Thread GitBox

ccaominh commented on a change in pull request #8825: Add note on JDBC libs for 
lookups
URL: https://github.com/apache/incubator-druid/pull/8825#discussion_r342773224
 
 

 ##
 File path: docs/development/extensions-core/lookups-cached-global.md
 ##
 @@ -368,6 +368,12 @@ The JDBC lookups will poll a database to populate its 
local cache. If the `tsCol
 }
 ```
 
+> If using JDBC, you will need to add your database's client JAR files to the 
extension's directory.
+> For MySQL, you can get it from https://dev.mysql.com/downloads/connector/j/, 
and for Postgres, from
+> https://jdbc.postgresql.org/download.html or from the 
`extensions/postgresql-metadata-storage/`.
 
 Review comment:
   Typo: from the `extensions/postgresql-metadata-storage/` -> the 
`extensions/postgresql-metadata-storage/` directory.
   
   Or: from the `extensions/postgresql-metadata-storage/` -> from 
`extensions/postgresql-metadata-storage/`


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] clintropolis commented on issue #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

clintropolis commented on issue #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419#issuecomment-550018795
 
 
   >Why is this a development blocker?
   
   The old version of `scrooge-maven-plugin` uses a super old forked version of 
libthrift for some reason that isn't on maven central, and seems to have 
recently stopped working for teamcity and lgtm CI checks, meaning no PRs are 
passing.


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] clintropolis commented on issue #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

clintropolis commented on issue #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419#issuecomment-550019038
 
 
   >This PR will make Travis failed because it's not updating licences.yaml.
   
   It actually might not, the old version doesn't appear to be in 
`licenses.yaml`.
   
   


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] jnaous commented on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

jnaous commented on issue #8822: optimize numeric column null value checking 
for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-550029100
 
 
   Sorry my question wasn't clear. I meant in terms of the cost of the full
   operation on a row. For example, if we're doing a longSum operation on a
   column, what would the cost per row be, and what would the percentage
   overhead of this check be?
   
   On Tue, Nov 5, 2019 at 12:02 PM Clint Wylie 
   wrote:
   
   > I'm curious what
   > percentage of the entire cost for processing a row a null check would be so
   > we can have a good idea of what % overhead we're talking about.
   >
   > The last 3 animations show the estimated per row cost in nanoseconds for
   > each of the 3 strategies. I will summarize:
   >
   >- get - most of the numbers look to be in the 10-25ns per row range
   >(higher at low selectivity where it matters most)
   >- IntIterator - about half are under 10ns per row (at low
   >selectivity), this is definitely the best, but at super high 
selectivities
   >(.1% of rows selected) with very dense bitmaps it climbs to over a 
couple
   >of microseconds per row
   >- PeekableIntIterator - about half are between 10-15ns per row (at low
   >selectivity), most below 25ns, but also has more overhead with dense
   >bitmaps at very high selectivity but only climbs to about 50-60ns per 
row
   >in the worst case.
   >
   > To me it kind of seems like a toss up to me which is better between using
   > the PeekableIntIterator and the plain IntIterator, it almost seems worth
   > it to eat the slow per row times at high selectivity in exchange for that
   > 5ns per row at low selectivity, but both approaches fair better at low
   > selectivity than using get, so I went more conservative and used the
   > PeekableIntIterator for now.
   >
   > —
   > You are receiving this because you commented.
   > Reply to this email directly, view it on GitHub
   > 
,
   > or unsubscribe
   > 

   > .
   >
   
   
   -- 
   Jad Naous
   Imply | VP R
   650-521-3425
   jad.na...@imply.io
   


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] clintropolis commented on issue #8824: teamcity build is failing with node module related issue

2019-11-05 Thread GitBox

clintropolis commented on issue #8824: teamcity build is failing with node 
module related issue
URL: 
https://github.com/apache/incubator-druid/issues/8824#issuecomment-550029229
 
 
   [Looking closer at that build 
log](https://teamcity.jetbrains.com/viewLog.html?buildId=2618987=OpenSourceProjects_Druid_InspectionsPullRequests=buildLog&_focus=12467),
 it looks like it's related to the same libthrift problem affecting all the 
other PRs which should be resolved by #8419.


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] richardstartin commented on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

richardstartin commented on issue #8822: optimize numeric column null value 
checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-549985610
 
 
   Cool heatmaps! Iteration should definitely perform better than calls to 
`contains` because it reduces the complexity of each call from logarithmic in 
the number of non empty 16 bit blocks to constant, obviously with a small set 
up cost. This is the same principle as #6764 which removes binary searches 
during bitmap construction.
   
   It looks like the operation in question 
(`VectorSelectorUtils.populateNullVector`) is actually simulating the 
extraction of a mask from the bitmap at the offset of the current vector. 
Perhaps, with the ability to skip to the next non empty vector (given a vector 
width), it would be quite easy to implement this as an iterator which returns 
masks on each call to `next`. Perhaps you could plug this in to Druid's 
vectorized query engine without needing to construct an iterator per vector. cc 
@lemire.


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] richardstartin edited a comment on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

richardstartin edited a comment on issue #8822: optimize numeric column null 
value checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-549985610
 
 
   Cool heatmaps! Iteration should definitely perform better than calls to 
`contains` because it reduces the complexity of each call from logarithmic in 
the number of non empty 16 bit blocks to constant, obviously with a small set 
up cost. This is the same principle as #6764 which removes binary searches 
during bitmap construction.
   
   It looks like the operation in question 
(`VectorSelectorUtils.populateNullVector`) is actually simulating the 
extraction of a mask from the bitmap at the offset of the current vector. 
Perhaps, with the ability to skip to the next non empty vector (given a vector 
width), it would be quite easy to implement this as an iterator which returns 
masks on each call to `next`. Perhaps you could plug this in to Druid's 
vectorized query engine. cc @lemire.


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] Fokko edited a comment on issue #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

Fokko edited a comment on issue #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419#issuecomment-550016704
 
 
   `scrooge-maven-plugin` is already using Thrift 0.10.0. I've rebased onto 
master.
   
   Why is this a development blocker?


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] clintropolis edited a comment on issue #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

clintropolis edited a comment on issue #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419#issuecomment-550019038
 
 
   >This PR will make Travis failed because it's not updating licences.yaml.
   
   It actually might not, the old version doesn't appear to be in 
`licenses.yaml`, though maybe it should be since there is a `NOTICE` file, 
https://github.com/apache/thrift/blob/master/NOTICE.
   
   


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] jnaous opened a new pull request #8825: Add note on JDBC libs for lookups

2019-11-05 Thread GitBox

jnaous opened a new pull request #8825: Add note on JDBC libs for lookups
URL: https://github.com/apache/incubator-druid/pull/8825
 
 
   Add documentation on needed lib files to make JDBC lookups work.
   


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] clintropolis commented on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

clintropolis commented on issue #8822: optimize numeric column null value 
checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-549995118
 
 
   >I'm curious what
   percentage of the entire cost for processing a row a null check would be so
   we can have a good idea of what % overhead we're talking about.
   
   The last 3 animations show the estimated per row cost in nanoseconds for 
each of the 3 strategies. I will summarize:
   * `get` - most of the numbers look to be in the 10-25ns per row range 
(higher at low selectivity where it matters most)
   * `IntIterator` - about half are under 10ns per row (at low selectivity), 
this is definitely the best, but at super high selectivities (.1% of rows 
selected) with very dense bitmaps it climbs to over a couple of microseconds 
per row
   * `PeekableIntIterator` - about half are between 10-15ns per row (at low 
selectivity), most below 25ns, but also has more overhead with dense bitmaps at 
very high selectivity but only climbs to about 50-60ns per row in the worst 
case.
   
   To me it kind of seems like a toss up to me which is better between using 
the `PeekableIntIterator` and the plain `IntIterator`, it almost seems worth it 
to eat the slow per row times at high selectivity in exchange for that 5ns per 
row at low selectivity, but both approaches fair better at low selectivity than 
using `get`, so I went more conservative and used the `PeekableIntIterator` for 
now.


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] LoriLori commented on issue #8808: use copy-on-write list in InMemoryAppender

2019-11-05 Thread GitBox

LoriLori commented on issue #8808: use copy-on-write list in InMemoryAppender
URL: https://github.com/apache/incubator-druid/pull/8808#issuecomment-550001537
 
 
   I am learning from better developers and was thinking about contract of 
logger and how would it could be solved should it be inmemorylogger be used 
somewhere else some day.
   Isn't copy on write an overhead and a bit teaching inappropriate use of copy 
on write?
   Isn't logEvents.toArray() atomic  appropriate? 
   return Collections.unmodifiableList(Arrays.asList((LogEvent[]) 
logEvents.toArray()));
   Code readability is lower and would require a comment maybe.
   


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] SEKIRO-J commented on a change in pull request #8748: Use earliest offset on kafka newly discovered partitions

2019-11-05 Thread GitBox

SEKIRO-J commented on a change in pull request #8748: Use earliest offset on 
kafka newly discovered partitions
URL: https://github.com/apache/incubator-druid/pull/8748#discussion_r342773057
 
 

 ##
 File path: 
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
 ##
 @@ -259,4 +260,36 @@ private static void wrapExceptions(Runnable runnable)
   return null;
 });
   }
+
+  @VisibleForTesting
+  private KafkaConsumer getKafkaConsumer(Map 
consumerConfigs)
+  {
+final Properties props = new Properties();
+addConsumerPropertiesFromConfig(props, sortingMapper, consumerProperties);
+props.putAll(consumerConfigs);
+
+ClassLoader currCtxCl = Thread.currentThread().getContextClassLoader();
+try {
+  
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
+  Deserializer keyDeserializerObject = getKafkaDeserializer(props, 
"key.deserializer");
+  Deserializer valueDeserializerObject = getKafkaDeserializer(props, 
"value.deserializer");
+
+  return new KafkaConsumer<>(props, keyDeserializerObject, 
valueDeserializerObject);
+}
+finally {
+  Thread.currentThread().setContextClassLoader(currCtxCl);
+}
+  }
+
+  @VisibleForTesting
+  public KafkaRecordSupplier(
+  Map consumerProperties,
+  ObjectMapper sortingMapper,
+  Map consumerConfigs
+  )
+  {
+this.consumerProperties = consumerProperties;
+this.sortingMapper = sortingMapper;
+this.consumer = getKafkaConsumer(consumerConfigs);
+  }
 
 Review comment:
   `getKafkaConsumer()` is a private method. And it depends on other parameters.


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] SEKIRO-J commented on a change in pull request #8748: Use earliest offset on kafka newly discovered partitions

2019-11-05 Thread GitBox

SEKIRO-J commented on a change in pull request #8748: Use earliest offset on 
kafka newly discovered partitions
URL: https://github.com/apache/incubator-druid/pull/8748#discussion_r342776259
 
 

 ##
 File path: 
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
 ##
 @@ -259,4 +260,36 @@ private static void wrapExceptions(Runnable runnable)
   return null;
 });
   }
+
+  @VisibleForTesting
+  private KafkaConsumer getKafkaConsumer(Map 
consumerConfigs)
+  {
+final Properties props = new Properties();
+addConsumerPropertiesFromConfig(props, sortingMapper, consumerProperties);
+props.putAll(consumerConfigs);
+
+ClassLoader currCtxCl = Thread.currentThread().getContextClassLoader();
+try {
+  
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
+  Deserializer keyDeserializerObject = getKafkaDeserializer(props, 
"key.deserializer");
+  Deserializer valueDeserializerObject = getKafkaDeserializer(props, 
"value.deserializer");
+
+  return new KafkaConsumer<>(props, keyDeserializerObject, 
valueDeserializerObject);
+}
+finally {
+  Thread.currentThread().setContextClassLoader(currCtxCl);
+}
+  }
+
+  @VisibleForTesting
+  public KafkaRecordSupplier(
+  Map consumerProperties,
+  ObjectMapper sortingMapper,
+  Map consumerConfigs
+  )
+  {
+this.consumerProperties = consumerProperties;
+this.sortingMapper = sortingMapper;
+this.consumer = getKafkaConsumer(consumerConfigs);
+  }
 
 Review comment:
`getKafkaConsumer ` depends on another private method,  
`addConsumerPropertiesFromConfig`, copy both method to test file?


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] jihoonson commented on issue #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

jihoonson commented on issue #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419#issuecomment-550010108
 
 
   +1 after CI.
   
   LGTM and Teamcity are failing because of the missing libthrift-0.5.0-1 jar. 
I think we can solve this by simply updating the version of 
`scrooge-maven-plugin` which is using that versino of libthrift.
   
   This PR will make Travis failed because it's not updating `licences.yaml`. I 
will raise a PR to fix that shortly.


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] Fokko commented on issue #8419: Bump Apache Thrift to 0.10.0

2019-11-05 Thread GitBox

Fokko commented on issue #8419: Bump Apache Thrift to 0.10.0
URL: https://github.com/apache/incubator-druid/pull/8419#issuecomment-550022104
 
 
   Thanks, updating scrooge should fix the dependency on the ancient version of 
libthrift indeed. Jenkins is running on my fork as well: 
https://travis-ci.org/Fokko/druid/builds/607854795 The one on Apache is still 
pending due to capacity issues.


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] clintropolis commented on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

clintropolis commented on issue #8822: optimize numeric column null value 
checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-549717755
 
 
   >The heatmaps look super cool! (although I don't think I fully understand 
them yet :| ) What did you use to build them?
   
   Hah, thanks, I used R with ggplot2 to make them. I'll try to clean up the 
code and attach it, if I have a chance, in case anyone else wants to do some 
benchmarking tinker with the results. As for what they mean, I'll try my best 
to explain it as succinctly as possible .
   
   The benchmark I added in this PR, 
`NullHandlingBitmapGetVsIteratorBenchmark`, is simulating approximately what 
happens during query processing on a historical for numerical null columns when 
used with something like a `NullableAggregator`, which is a wrapper around 
another `Aggregator` to ignore `null` values or delegate aggregation to the 
wrapped aggregator for rows that have actual values.
   
   When SQL compatible null handling is enabled, numeric columns are stored 
with 2 parts if nulls are present: the column itself, and a bitmap that has a 
set bit for each null value. At query time, filters are evaluated to compute 
something called an `Offset`, which is basically just the set of rows that are 
taking part in the query, and are used to create a column value/vector selector 
for those rows from the underlying column. Selectors have a `isNull` method 
which can be used to determine if a particular row is a `null`, and for numeric 
columns this is checking if that row is set on the bitmap. So mechanically, 
`NullableAggregator` will check each row from the selector to see if it is null 
(through the underlying bitmap), if it is, ignore it, but if not, delegate to 
the underlying `Aggregator` to do whatever it does to compute the result.
   
   The benchmark simplifies this concept into using a `BitSet` to simulate the 
`Offset`, an `ImmutableBitmap` for the null value bitmap, and a for loop that 
iterates over the "rows" selected by the `BitSet` to emulate the behavior of 
the aggregator on the selector, checking for set bits in the `ImmutableBitmap` 
for each index like `isNull` would be doing.
   
   Translating this into heatmap, the y axis is showing the effects of 
differences in density of the null bitmap (bottom is a few null values, top is 
nearly all rows are null), the x axis is the differences in the number of rows 
that our selector will select (left side selects very few rows, right scans 
nearly all rows), and the z axis is the difference in benchmark operation time 
between using bitmap.get` and using an iterator (or peekable iterator) from the 
null bitmap to move along with the iterator on the selectivity bitset. Further, 
some of the heatmaps have translated the raw benchmark times into the _time per 
row_ by scaling the time by how many rows are selected, to standardize 
measurement across the x axis, making it easier to compare the 2 strategies.
   
   Sorry, that didn't end up being so short... I .. hope this didn't make it 
more confusing  


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] clintropolis commented on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

clintropolis commented on issue #8822: optimize numeric column null value 
checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-549718415
 
 
   To follow-up on the PR description, I let the concise benchmarks finish 
where nearly all of the rows are selected:
   ```
   Benchmark  (bitmapType)  
(filterMatch)  (nullDensity)  (numRows)  Mode  Cnt  Score   Error  Units
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
  0.9  0 50  avgt2   2438.318  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
  0.90.1 50  avgt2  261150067.426  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
  0.9   0.25 50  avgt2  430133586.339  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
  0.90.5 50  avgt2  623322588.940  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
  0.9   0.75 50  avgt2  449875260.568  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
  0.9   0.99 50  avgt2   29015358.505  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise  
  0.9  0 50  avgt2   2316.307  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise  
  0.90.1 50  avgt2   4158.871  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise  
  0.9   0.25 50  avgt2   6041.184  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise  
  0.90.5 50  avgt2   8397.707  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise  
  0.9   0.75 50  avgt2   6129.792  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise  
  0.9   0.99 50  avgt2   3511.961  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise  
  0.9  0 50  avgt2   2440.615  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise  
  0.90.1 50  avgt2   4431.640  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise  
  0.9   0.25 50  avgt2   6302.718  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise  
  0.90.5 50  avgt2   8911.671  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise  
  0.9   0.75 50  avgt2   6809.052  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise  
  0.9   0.99 50  avgt2   4995.936  us/op
   ```
   
   Using `get` with concise is just terribad. The results for 1% of rows is 
similarly pretty awful, though a couple orders of magnitude less bad:
   ```
   Benchmark  (bitmapType)  
(filterMatch)  (nullDensity)  (numRows)  Mode  CntScore   Error  Units
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
 0.01  0 50  avgt2  100.110  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
 0.010.1 50  avgt2  3878501.478  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
 0.01   0.25 50  avgt2  4853693.538  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
 0.010.5 50  avgt2  8399206.128  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
 0.01   0.75 50  avgt2  5169724.280  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.getconcise  
 0.01   0.99 50  avgt2   440739.562  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise  
 0.01  0 50  avgt2   69.756  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise  
 0.010.1 50  avgt2 1813.610  us/op
   NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise  
 0.01

[GitHub] [incubator-druid] clintropolis edited a comment on issue #8578: parallel broker merges on fork join pool

2019-11-05 Thread GitBox

clintropolis edited a comment on issue #8578: parallel broker merges on fork 
join pool
URL: https://github.com/apache/incubator-druid/pull/8578#issuecomment-54976
 
 
   ### simulated heavy load
   I collected another round of data using the same benchmarks as my 'more 
realistic worst case' comment, but this time plotting what happens when a large 
number of queries all start within a 500ms spread, which might be a more 
typical heavy load, rather than simulating a large concurrent spike of 
simultaneous queries like the last set of results.
   
   In this scenario, parallel merges outperform the same threaded merges until 
much higher concurrency than the concurrent spike model. This is at least 
partially driven by the fact that each individual thread can make a better 
estimate about utilization than is possible in the spike model.
   
   # 'small' sequences
   
![thread-groups-typical-distribution-small-500ms](https://user-images.githubusercontent.com/1577461/68194001-09a57c80-ff69-11e9-9325-d3e70b4853d2.gif)
   
   # 'moderately large' sequences
   
![thread-groups-typical-distribution-moderately-large-500ms](https://user-images.githubusercontent.com/1577461/68193956-f7c3d980-ff68-11e9-9d29-d3ee688d8cb6.gif)
   
   
   # overall average
   
![thread-groups-typical-distribution-average-500ms](https://user-images.githubusercontent.com/1577461/68194026-132ee480-ff69-11e9-9601-921a98188feb.gif)
   
   I think future work could focus on making the concurrent spike behavior a 
bit more chillax through a variety of means, but I find these results to be 
'good enough' for now. 
   
   Anyone want to see any other scenarios?


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] clintropolis commented on issue #8578: parallel broker merges on fork join pool

2019-11-05 Thread GitBox

clintropolis commented on issue #8578: parallel broker merges on fork join pool
URL: https://github.com/apache/incubator-druid/pull/8578#issuecomment-54976
 
 
   ### simulated heavy load
   I collected another round of data using the same benchmarks as my 'more 
realistic worst case' comment, but this time plotting what happens when a large 
number of queries all start within a 500ms spread, which might be a more 
typical heavy load, rather than simulating a large concurrent spike of 
simultaneous queries like the last set of results.
   
   In this scenario, parallel merges outperform the same threaded merges until 
much higher concurrency than the concurrent spike model.
   
   # 'small' sequences
   
![thread-groups-typical-distribution-small-500ms](https://user-images.githubusercontent.com/1577461/68194001-09a57c80-ff69-11e9-9325-d3e70b4853d2.gif)
   
   # 'moderately large' sequences
   
![thread-groups-typical-distribution-moderately-large-500ms](https://user-images.githubusercontent.com/1577461/68193956-f7c3d980-ff68-11e9-9d29-d3ee688d8cb6.gif)
   
   
   # overall average
   
![thread-groups-typical-distribution-average-500ms](https://user-images.githubusercontent.com/1577461/68194026-132ee480-ff69-11e9-9601-921a98188feb.gif)
   
   I think future work could focus on making the concurrent spike behavior a 
bit more chillax through a variety of means, but I find these results to be 
'good enough' for now. 
   
   Anyone want to see any other scenarios?


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] jnaous commented on issue #8822: optimize numeric column null value checking for low filter selectivity (more rows)

2019-11-05 Thread GitBox

jnaous commented on issue #8822: optimize numeric column null value checking 
for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#issuecomment-549866189
 
 
   Thanks Clint! These /are/ really good benchmarks. I'm curious what
   percentage of the entire cost for processing a row a null check would be so
   we can have a good idea of what % overhead we're talking about.
   
   
   On Tue, Nov 5, 2019 at 12:36 AM Clint Wylie 
   wrote:
   
   > To follow-up on the PR description, I let the concise benchmarks finish
   > where nearly all of the rows are selected:
   >
   > Benchmark  (bitmapType)  
(filterMatch)  (nullDensity)  (numRows)  Mode  Cnt  Score   Error  Units
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
0.9  0 50  avgt2   2438.318  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
0.90.1 50  avgt2  261150067.426  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
0.9   0.25 50  avgt2  430133586.339  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
0.90.5 50  avgt2  623322588.940  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
0.9   0.75 50  avgt2  449875260.568  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
0.9   0.99 50  avgt2   29015358.505  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise
0.9  0 50  avgt2   2316.307  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise
0.90.1 50  avgt2   4158.871  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise
0.9   0.25 50  avgt2   6041.184  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise
0.90.5 50  avgt2   8397.707  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise
0.9   0.75 50  avgt2   6129.792  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.iterator   concise
0.9   0.99 50  avgt2   3511.961  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise
0.9  0 50  avgt2   2440.615  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise
0.90.1 50  avgt2   4431.640  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise
0.9   0.25 50  avgt2   6302.718  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise
0.90.5 50  avgt2   8911.671  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise
0.9   0.75 50  avgt2   6809.052  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.peekableIterator   concise
0.9   0.99 50  avgt2   4995.936  us/op
   >
   > Using get with concise is just terribad. The results for 1% of rows is
   > similarly pretty awful, though a couple orders of magnitude less bad:
   >
   > Benchmark  (bitmapType)  
(filterMatch)  (nullDensity)  (numRows)  Mode  CntScore   Error  Units
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
   0.01  0 50  avgt2  100.110  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
   0.010.1 50  avgt2  3878501.478  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
   0.01   0.25 50  avgt2  4853693.538  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
   0.010.5 50  avgt2  8399206.128  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
   0.01   0.75 50  avgt2  5169724.280  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.getconcise
   0.01   0.99 50  avgt2   440739.562  us/op
   > NullHandlingBitmapGetVsIteratorBenchmark.iterator

82 matches

Mail list logo