You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/06/03 20:54:11 UTC
[GitHub] [druid] clintropolis opened a new pull request #9982: make joinables closeable
clintropolis opened a new pull request #9982:
URL: https://github.com/apache/druid/pull/9982
### Description
This PR makes `Joinable`, `JoinClause`, and `IndexedTable` implement `Closeable`, so that a future PR can add reference counting for `IndexedTable` that are backed by a `Segment` described in proposal #9953. `HashJoinSegment` now wraps a `ReferenceCountingSegment`, allowing both the segment reference count to be incremented/decremented when used, and also closing the associated list of `JoinClause` when the references to the segment are released.
`ReferenceCountingSegment` has been refactored to extend a new base type, `ReferenceCountingClosableObject`, which handles the actual reference counting part and prepare for `ReferenceCountingIndexedTable` (which does not exist yet).
A new interface, `SegmentReference` has been added to capture a `Segment` which has references which must be acquired and released:
```
public interface SegmentReference extends Segment
{
Optional<Closer> acquireReferences();
}
```
The contract of the closer is that if the call to `ReferenceCountingClosableObject#increment` is successful, then a closer will be present in the `Optional`, which when closed will call `ReferenceCountingClosableObject#decrement` on any tracked references. `ReferenceCountingSegment` and `HashJoinSegment` implement this interface, and `ReferenceCountingSegmentQueryRunner` now operates on this interface instead of directly using `Segment` and a `ReferenceCounter`.
Finally, I have marked `AbstractSegment` as deprecated, and moved it's only functionality into a default method on the interface to make working with `Segment` a bit easier. I have not deleted `AbstractSegment` yet, it's just an empty husk that we can remove in a future release on the off chance that any custom extensions are relying on this class.
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/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/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.
<hr>
##### Key changed/added classes in this PR
* `ReferenceCountingSegmentQueryRunner`
* `ReferenceCountingClosableObject`
* `ReferenceCountingSegment`
* `HashJoinSegment`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9982:
URL: https://github.com/apache/druid/pull/9982#discussion_r437755142
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegment.java
##########
@@ -98,4 +101,30 @@ public void close() throws IOException
{
baseSegment.close();
}
+
+ @Override
+ public Optional<Closeable> acquireReferences()
+ {
+ Closer closer = Closer.create();
+ boolean acquireFailed = baseSegment.acquireReferences().map(closeable -> {
+ closer.register(closeable);
+ return false;
+ }).orElse(true);
+
+ for (JoinableClause claws : clauses) {
+ if (acquireFailed) {
+ break;
+ }
+ acquireFailed |= claws.acquireReferences().map(closeable -> {
Review comment:
modified javadoc to more clearly explain the contract
##########
File path: processing/src/main/java/org/apache/druid/segment/join/table/IndexedTable.java
##########
@@ -30,8 +31,11 @@
*
* The main user of this class is {@link IndexedTableJoinable}, and its main purpose is to participate in joins.
*/
-public interface IndexedTable
+public interface IndexedTable extends ReferenceCountedObject
{
+ @SuppressWarnings("unused")
+ String version();
Review comment:
added
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9982:
URL: https://github.com/apache/druid/pull/9982#discussion_r437755823
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/QueryMaker.java
##########
@@ -126,7 +126,7 @@ public ObjectMapper getJsonMapper()
return DataSourceAnalysis.forDataSource(query.getDataSource())
.getBaseQuerySegmentSpec()
.map(QuerySegmentSpec::getIntervals)
- .orElse(query.getIntervals());
+ .orElseGet(query::getIntervals);
Review comment:
I'm not sure a comment is necessary... I think we should probably always use `orElseGet` if the 'else' isn't a primitive because `Optional` must eagerly evaluate whatever goes in there. This should maybe be done with some sort of static analysis, but I couldn't figure it out in like the 2 minutes I devoted to it so far.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] lgtm-com[bot] commented on pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #9982:
URL: https://github.com/apache/druid/pull/9982#issuecomment-640974350
This pull request **introduces 1 alert** when merging 7c69e1b85fc9b2ca77d33a7f2e2ecf015976343e into ee7bda5d8a7e68692cf29182bdbb5dcbe29400f4 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-9f37c2455acd1620fa52d42d0c197a1fb12529cc)
**new 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on a change in pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #9982:
URL: https://github.com/apache/druid/pull/9982#discussion_r436240145
##########
File path: processing/src/main/java/org/apache/druid/segment/ReferenceCountedObject.java
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.segment;
+
+import org.apache.druid.java.util.common.io.Closer;
+
+import java.io.Closeable;
+import java.util.Optional;
+
+/**
+ * Interface for an object that may have a reference acquired in the form of a {@link Closeable}. This is intended to be
+ * used with an implementation of {@link ReferenceCountingCloseableObject}, or anything else that wishes to provide
+ * a method to account for the acquire and release of a reference to the object.
+ */
+public interface ReferenceCountedObject
+{
+ /**
+ * This method is expected to increment a reference count and provide a {@link Closer} that decrements the reference
Review comment:
I think `Closer` should be `Closeable` here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9982:
URL: https://github.com/apache/druid/pull/9982#discussion_r437755117
##########
File path: processing/src/main/java/org/apache/druid/segment/ReferenceCountingCloseableObject.java
##########
@@ -0,0 +1,144 @@
+/*
+ * 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.segment;
+
+import org.apache.druid.java.util.common.io.Closer;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.Closeable;
+import java.util.Optional;
+import java.util.concurrent.Phaser;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * ReferenceCountingCloseableObject implements something like automatic reference count-based resource management,
+ * backed by a {@link Phaser}.
+ *
+ * ReferenceCountingCloseableObject allows consumers to call {@link #close()} before some other "users", which called
+ * {@link #increment()} or {@link #incrementReferenceAndDecrementOnceCloseable()}, but have not called
+ * {@link #decrement()} yet or the closer for {@link #incrementReferenceAndDecrementOnceCloseable()}, and the wrapped
+ * object won't be actually closed until that all references are released.
+ */
+public abstract class ReferenceCountingCloseableObject<BaseObject extends Closeable> implements Closeable
+{
+ private static final EmittingLogger log = new EmittingLogger(ReferenceCountingCloseableObject.class);
Review comment:
Ah, I just copied this from `ReferenceCountingSegment`, changed.
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegment.java
##########
@@ -98,4 +101,30 @@ public void close() throws IOException
{
baseSegment.close();
}
+
+ @Override
+ public Optional<Closeable> acquireReferences()
+ {
+ Closer closer = Closer.create();
+ boolean acquireFailed = baseSegment.acquireReferences().map(closeable -> {
+ closer.register(closeable);
+ return false;
+ }).orElse(true);
+
+ for (JoinableClause claws : clauses) {
+ if (acquireFailed) {
+ break;
+ }
+ acquireFailed |= claws.acquireReferences().map(closeable -> {
Review comment:
modified javadoc to tell
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9982:
URL: https://github.com/apache/druid/pull/9982#discussion_r437635575
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/QueryMaker.java
##########
@@ -126,7 +126,7 @@ public ObjectMapper getJsonMapper()
return DataSourceAnalysis.forDataSource(query.getDataSource())
.getBaseQuerySegmentSpec()
.map(QuerySegmentSpec::getIntervals)
- .orElse(query.getIntervals());
+ .orElseGet(query::getIntervals);
Review comment:
Probably worth commenting why it should be lazy. Same for other `orElseGet()`.
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegment.java
##########
@@ -98,4 +101,30 @@ public void close() throws IOException
{
baseSegment.close();
}
+
+ @Override
+ public Optional<Closeable> acquireReferences()
+ {
+ Closer closer = Closer.create();
+ boolean acquireFailed = baseSegment.acquireReferences().map(closeable -> {
+ closer.register(closeable);
+ return false;
+ }).orElse(true);
+
+ for (JoinableClause claws : clauses) {
+ if (acquireFailed) {
+ break;
+ }
+ acquireFailed |= claws.acquireReferences().map(closeable -> {
Review comment:
Hmm, should the contract of `acquireReferences()` clarify that this method should never throw an exception? Otherwise, we should make sure the `closer` will be safely closed on exceptions.
##########
File path: processing/src/main/java/org/apache/druid/segment/ReferenceCountingCloseableObject.java
##########
@@ -0,0 +1,144 @@
+/*
+ * 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.segment;
+
+import org.apache.druid.java.util.common.io.Closer;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.Closeable;
+import java.util.Optional;
+import java.util.concurrent.Phaser;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * ReferenceCountingCloseableObject implements something like automatic reference count-based resource management,
+ * backed by a {@link Phaser}.
+ *
+ * ReferenceCountingCloseableObject allows consumers to call {@link #close()} before some other "users", which called
+ * {@link #increment()} or {@link #incrementReferenceAndDecrementOnceCloseable()}, but have not called
+ * {@link #decrement()} yet or the closer for {@link #incrementReferenceAndDecrementOnceCloseable()}, and the wrapped
+ * object won't be actually closed until that all references are released.
+ */
+public abstract class ReferenceCountingCloseableObject<BaseObject extends Closeable> implements Closeable
+{
+ private static final EmittingLogger log = new EmittingLogger(ReferenceCountingCloseableObject.class);
Review comment:
nit: it's emitting nothing and can be a `Logger`.
##########
File path: processing/src/main/java/org/apache/druid/segment/join/table/IndexedTable.java
##########
@@ -30,8 +31,11 @@
*
* The main user of this class is {@link IndexedTableJoinable}, and its main purpose is to participate in joins.
*/
-public interface IndexedTable
+public interface IndexedTable extends ReferenceCountedObject
{
+ @SuppressWarnings("unused")
+ String version();
Review comment:
nit: Javadoc?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9982:
URL: https://github.com/apache/druid/pull/9982#discussion_r437756683
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/QueryMaker.java
##########
@@ -126,7 +126,7 @@ public ObjectMapper getJsonMapper()
return DataSourceAnalysis.forDataSource(query.getDataSource())
.getBaseQuerySegmentSpec()
.map(QuerySegmentSpec::getIntervals)
- .orElse(query.getIntervals());
+ .orElseGet(query::getIntervals);
Review comment:
btw, I made the changes here after I made the mistake of not using `orElseGet` in `ReferenceCountingSegmentQueryRunner` and funny side-effects were happening causing integration tests to fail, so I looked for other usages of `orElse` and switched the ones that were not returning a primitive or static value to use `orElseGet`, of which there were only these 2 I think.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9982:
URL: https://github.com/apache/druid/pull/9982#discussion_r437125850
##########
File path: processing/src/main/java/org/apache/druid/segment/ReferenceCountedObject.java
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.segment;
+
+import org.apache.druid.java.util.common.io.Closer;
+
+import java.io.Closeable;
+import java.util.Optional;
+
+/**
+ * Interface for an object that may have a reference acquired in the form of a {@link Closeable}. This is intended to be
+ * used with an implementation of {@link ReferenceCountingCloseableObject}, or anything else that wishes to provide
+ * a method to account for the acquire and release of a reference to the object.
+ */
+public interface ReferenceCountedObject
+{
+ /**
+ * This method is expected to increment a reference count and provide a {@link Closer} that decrements the reference
Review comment:
Oops, thanks, was previously using `Closer` but changed and didn't fixup javadoc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #9982:
URL: https://github.com/apache/druid/pull/9982#issuecomment-641693650
Thanks for review @jon-wei and @jihoonson
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #9982: make joinables closeable
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #9982:
URL: https://github.com/apache/druid/pull/9982
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org