New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IGNITE-10789 Correct errors processing in CacheInterceptor with thin … #8169
Conversation
@@ -115,7 +116,7 @@ | |||
int status = e instanceof IgniteClientException ? | |||
((IgniteClientException)e).statusCode() : ClientStatus.FAILED; | |||
|
|||
return new ClientResponse(req.requestId(), status, e.getMessage()); | |||
return new ClientResponse(req.requestId(), status, X.getFullStackTrace(e)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not include full stack traces into thin client responses. Thin client users are supposed to check server logs (if they have access) to understand the issue when error is returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disagree here, previous message (without my changes) has zero info for error detection, check it plz!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the message should be fixed. We absolutely can not pass server stack traces to the clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have conversions or something else about thin client return messages ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frequently users have no access to server logs, my fix helps to understand problem correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see this discussion: http://apache-ignite-developers.2346864.n4.nabble.com/Exception-handling-in-thin-client-should-we-pass-stack-traces-to-the-client-td22392.html
If we want to expose server stack traces to thin clients, this should be configurable and disabled by default.
modules/core/src/test/java/org/apache/ignite/client/IgniteBinaryTest.java
Show resolved
Hide resolved
cache.put(1, new ThinBinaryValue()); | ||
} | ||
catch (Exception e) { | ||
assertTrue(X.getFullStackTrace(e).contains(castErr)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are mostly interested that exception message makes sense. Top-level exception should be easy to understand. Please add an assertion for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current error message has a lot of inner info, yes we talking about it, if i correctly understand you suggest to add additional exception class and check it in all exception path places ? I don`t like this approach, or i miss something, check current exception path :
org.apache.ignite.cache.CachePartialUpdateException: Failed to update keys (retry update if possible).: [1]
at org.apache.ignite.internal.processors.cache.GridCacheUtils.convertToCacheException(GridCacheUtils.java:1252)
at org.apache.ignite.internal.processors.cache.IgniteCacheProxyImpl.cacheException(IgniteCacheProxyImpl.java:2083)
...
at java.lang.Thread.run(Thread.java:748)
Caused by: class org.apache.ignite.internal.processors.cache.CachePartialUpdateCheckedException: Failed to update keys (retry update if possible).: [1]
at org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridNearAtomicAbstractUpdateFuture.onPrimaryError(GridNearAtomicAbstractUpdateFuture.java:397)
at org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridNearAtomicSingleUpdateFuture.onPrimaryResponse(GridNearAtomicSingleUpdateFuture.java:253)
... 13 more
Suppressed: class org.apache.ignite.IgniteCheckedException: Failed to update keys.
at org.apache.ignite.internal.processors.cache.distributed.dht.atomic.UpdateErrors.addFailedKey(UpdateErrors.java:108)
at org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridNearAtomicUpdateResponse.addFailedKey(GridNearAtomicUpdateResponse.java:327)
at org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridDhtAtomicCache.updateSingle(GridDhtAtomicCache.java:2808)
at org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridDhtAtomicCache.update(GridDhtAtomicCache.java:2128)
at org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridDhtAtomicCache.updateAllAsyncInternal0(GridDhtAtomicCache.java:1945)
... 23 more
Suppressed: class org.apache.ignite.IgniteCheckedException: Runtime failure on search row: SearchRow [key=KeyCacheObjectImpl [part=1, val=1, hasValBytes=true], hash=1, cacheId=0]
at org.apache.ignite.internal.processors.cache.persistence.tree.BPlusTree.invoke(BPlusTree.java:1928)
at org.apache.ignite.internal.processors.cache.IgniteCacheOffheapManagerImpl$CacheDataStoreImpl.invoke0(IgniteCacheOffheapManagerImpl.java:1712)
at org.apache.ignite.internal.processors.cache.IgniteCacheOffheapManagerImpl$CacheDataStoreImpl.invoke(IgniteCacheOffheapManagerImpl.java:1695)
at org.apache.ignite.internal.processors.cache.IgniteCacheOffheapManagerImpl.invoke(IgniteCacheOffheapManagerImpl.java:444)
at org.apache.ignite.internal.processors.cache.GridCacheMapEntry.innerUpdate(GridCacheMapEntry.java:2319)
at org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridDhtAtomicCache.updateSingle(GridDhtAtomicCache.java:2667)
... 25 more
Caused by: class org.apache.ignite.IgniteCheckedException: org.apache.ignite.internal.binary.BinaryObjectImpl cannot be cast to org.apache.ignite.client.IgniteBinaryTest$ThinBinaryValue
at org.apache.ignite.internal.processors.cache.GridCacheMapEntry$AtomicCacheUpdateClosure.update(GridCacheMapEntry.java:6379)
at org.apache.ignite.internal.processors.cache.GridCacheMapEntry$AtomicCacheUpdateClosure.call(GridCacheMapEntry.java:6174)
at org.apache.ignite.internal.processors.cache.GridCacheMapEntry$AtomicCacheUpdateClosure.call(GridCacheMapEntry.java:5863)
at org.apache.ignite.internal.processors.cache.persistence.tree.BPlusTree$Invoke.invokeClosure(BPlusTree.java:3994)
at org.apache.ignite.internal.processors.cache.persistence.tree.BPlusTree$Invoke.access$5700(BPlusTree.java:3888)
at org.apache.ignite.internal.processors.cache.persistence.tree.BPlusTree.invokeDown(BPlusTree.java:2014)
at org.apache.ignite.internal.processors.cache.persistence.tree.BPlusTree.invoke(BPlusTree.java:1898)
... 30 more
Caused by: java.lang.ClassCastException: org.apache.ignite.internal.binary.BinaryObjectImpl cannot be cast to org.apache.ignite.client.IgniteBinaryTest$ThinBinaryValue
at org.apache.ignite.client.IgniteBinaryTest$ThinBinaryValueInterceptor.onBeforePut(IgniteBinaryTest.java:176)
at org.apache.ignite.internal.processors.cache.GridCacheMapEntry$AtomicCacheUpdateClosure.update(GridCacheMapEntry.java:6376)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, top-level exception should say "Failed to invoke cache interceptor of type T: ClassCastException: org.apache.ignite.internal.binary.BinaryObjectImpl cannot be cast to org.apache.ignite.client.IgniteBinaryTest$ThinBinaryValue".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this case is hardly to realize as i can see, i need additional custom Exception class and its further processing looks like stub and over engineering here plus if cache.put would have different errors on different nodes need some merge logic, thus i have no clear solution here, besides this part https://github.com/apache/ignite/pull/8169/files#diff-b877f1ecebf42e9839a94843abe445aeR119 hack, i can show only low level messages but seems it a stub too.
…client payload.
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summary
whereXXXX
- number of JIRA issue.(see the Maintainers list)
the
green visa
attached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.