zstan commented on code in PR #13089:
URL: https://github.com/apache/ignite/pull/13089#discussion_r3186687544
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/RootNode.java:
##########
@@ -324,9 +321,7 @@ private void checkException() {
if (e == null)
return;
- if (e instanceof IgniteSQLException)
- throw (IgniteSQLException)e;
- else
- throw new IgniteSQLException("An error occurred while query
executing - " + e.getMessage(), IgniteQueryErrorCode.UNKNOWN, e);
+ throw new IgniteSQLException("An error occurred while query executing
- " + e.getMessage(),
+ e instanceof IgniteSQLException ?
((IgniteSQLException)e).statusCode() : IgniteQueryErrorCode.UNKNOWN, e);
Review Comment:
probably not to re-pack but use approach like :
```
@Override public boolean hasNext() {
...
sneakyThrow(e);
}
public static <E extends Throwable> E sneakyThrow(Throwable e) throws E {
throw (E) e;
}
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/RootNode.java:
##########
@@ -205,8 +204,6 @@ public UUID queryId() {
/** {@inheritDoc} */
@Override public boolean hasNext() {
- checkException();
Review Comment:
why this check need to be removed ?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ClosableIteratorsHolder.java:
##########
@@ -97,18 +97,30 @@ private Reference nextRef(boolean blocking) {
}
/** */
- private AutoCloseable closeable(Object referent, Object resource) {
- if (!(resource instanceof AutoCloseable))
+ private AutoCloseable closeable(Object referent, Object rsrc) {
Review Comment:
```suggestion
@Nullable private AutoCloseable closeable(Object referent, Iterator<?>
rsrc) {
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ClosableIteratorsHolder.java:
##########
@@ -119,37 +131,66 @@ private DelegatingIterator(Iterator<T> delegate) {
/** {@inheritDoc} */
@Override public boolean hasNext() {
- return delegate.hasNext();
+ inUse = true;
+
+ try {
+ return delegate.hasNext();
+ }
+ finally {
+ inUse = false;
+ }
}
/** {@inheritDoc} */
@Override public T next() {
- return delegate.next();
+ inUse = true;
+
+ try {
+ return delegate.next();
+ }
+ finally {
+ inUse = false;
+ }
}
/** {@inheritDoc} */
@Override public void remove() {
- delegate.remove();
+ inUse = true;
+
+ try {
+ delegate.remove();
+ }
+ finally {
+ inUse = false;
+ }
}
/** {@inheritDoc} */
@Override public void forEachRemaining(Consumer<? super T> action) {
- delegate.forEachRemaining(action);
+ inUse = true;
+
+ try {
+ delegate.forEachRemaining(action);
+ }
+ finally {
+ inUse = false;
+ }
}
/** {@inheritDoc} */
@Override public void close() throws Exception {
+ if (log.isDebugEnabled())
+ log.debug("Closing iterator [delegate=" + delegate + ",
inUse=" + inUse + ']');
+
Commons.close(closeable);
}
}
/** */
private final class CloseableReference extends WeakReference implements
AutoCloseable {
/** */
- private CloseableReference(Object referent, Object resource) {
+ private CloseableReference(Object referent) {
Review Comment:
probably we need to remove WeakReference related code and create more
deterministoc model ? there is also no weak ref in ignite3
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ClosableIteratorsHolder.java:
##########
@@ -97,18 +97,30 @@ private Reference nextRef(boolean blocking) {
}
/** */
- private AutoCloseable closeable(Object referent, Object resource) {
- if (!(resource instanceof AutoCloseable))
+ private AutoCloseable closeable(Object referent, Object rsrc) {
+ if (!(rsrc instanceof AutoCloseable))
return null;
- return new CloseableReference(referent, resource);
+ CloseableReference ref = new CloseableReference(referent);
+
+ refMap.put(ref, rsrc);
+
+ return ref;
}
/** */
private final class DelegatingIterator<T> implements Iterator<T>,
AutoCloseable {
/** */
private final Iterator<T> delegate;
+ /**
+ * This variable is required to keep reference to current instance
while delegate call
+ * (hasNext/next/remove/forEachRemaining) is not completed. We
actually don't care about variable value and
+ * thread safety, it's only to prevent premature garbage collection
and iterator closing before returning
+ * result to the user.
+ */
+ private boolean inUse;
Review Comment:
debug related flag in production code - looks weird for me
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ClosableIteratorsHolder.java:
##########
@@ -97,18 +97,30 @@ private Reference nextRef(boolean blocking) {
}
/** */
- private AutoCloseable closeable(Object referent, Object resource) {
- if (!(resource instanceof AutoCloseable))
+ private AutoCloseable closeable(Object referent, Object rsrc) {
+ if (!(rsrc instanceof AutoCloseable))
return null;
- return new CloseableReference(referent, resource);
+ CloseableReference ref = new CloseableReference(referent);
+
+ refMap.put(ref, rsrc);
+
+ return ref;
}
/** */
private final class DelegatingIterator<T> implements Iterator<T>,
AutoCloseable {
/** */
private final Iterator<T> delegate;
+ /**
+ * This variable is required to keep reference to current instance
while delegate call
+ * (hasNext/next/remove/forEachRemaining) is not completed. We
actually don't care about variable value and
+ * thread safety, it's only to prevent premature garbage collection
and iterator closing before returning
+ * result to the user.
+ */
+ private boolean inUse;
Review Comment:
also i wonder ) i just comment this: //private boolean inUse;
and get already known : Suppressed: [CIRCULAR REFERENCE:
seems this flag mask it somehow ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
