You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "Bill (GitHub)" <gi...@apache.org> on 2018/10/16 18:29:49 UTC
[GitHub] [geode] Bill opened pull request #2625: GEODE-5568: Increase
wait in testCacheOpAfterQueryCancel()
Fix intermittently-failing test by increasing timeout
to account for GC pauses.
Co-authored-by: Bill Burcham <bb...@pivotal.io>
Co-authored-by: Ryan McMahon <rm...@pivotal.io>
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [] Have you written or updated unit tests to verify your changes?
- [] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt closed pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
[ pull request closed by mcmellawatt ]
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "Bill (GitHub)" <gi...@apache.org>.
I think this thread pool constructor has to change so that its thread(s) is a `LoggingThread`
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on issue #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Creating a new PR to avoid the clutter in this one. Also this one was approved for a fix which has since been removed.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
javadoc from this internal class won't show up in distro
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Extra white space
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
Also, the delayed method will change for each value as it gets compared since you will be using System.currentTimeMillis(). There might be some weird issues with that?
Side note:
The old way was that each query was inserted and the oldest was always at the head. We will still be adding queries in order.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
This is a valid point. As part of this commit, we deleted the code that removes the entries from the queue after the query completes, because we wanted the QueryMonitor to be the sole entity which empties the queue for simplicity. I think it is a valid concern that the queue may grow unbounded in the case where there are many short lived queries and a long expiration time. We can solve this by adding the remove logic back in when the query completes. We will also have to add equals() and hashcode() for removal to work properly.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Use assertThatThrownBy() from JUnit or AssertJ
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
This shouldn't be an issue - the `Delayed` interface on the `QueryExpiration` object implements `compareTo`, which will put the oldest candidates (most eligible for expiration) at the front of the queue.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Maybe more explicit variable names so its easier to understand at a glance?
newLowMemory
and
oldLowMemory = QueryMonitor.LOW_MEMORY
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "Bill (GitHub)" <gi...@apache.org>.
There is a performance tradeoff. In the old code, when a query was complete, a O(N) operation was performed immediately, to remove the query from the queue (where N is the number of queue entries).
So old:
- space: queue contains only queries that have not yet completed
- time: at query completion: O(N) removal from queue
new:
- space: more queue entries—some entries are for queries that have completed
- time: at query completion: **no** O(N) removal of query; O(1) processing of completed entry at head of queue
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Comment may be misleading now.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Rename to loadGenerationExecutorService
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Cancelled -> canceled
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
add `Objects.requireNonNull(cache)` check in constructor
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Due capitalized?
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Geode
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
Will this the handle scenario where we have completed the query, but the timeout is a very large timeout. Would this leave those query tasks in the queue until they completely expire, which could be a lot later?
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Delete these two lines (commented out code)
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Rename to cancelationTask.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "Bill (GitHub)" <gi...@apache.org>.
I think this thread pool constructor (`newScheduledThreadPool()`) has to change so that its thread(s) is a `LoggingThread`. Should call the overload that takes a factory.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Same as above.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "Bill (GitHub)" <gi...@apache.org>.
Latest commit restores the immediate removal of the query from the queue as soon as the query completes. This addresses @jhuynh1's concern about queue growth when `MAX_QUERY_EXECUTION_TIME` is long.
We also ran some performance numbers that indicate that the new `DelayQueue` performs at least as well as the old `ConcurrentLinkedQueue`-based implementation did. We looked at time to add queries, then remove them (in the same order, and in the reverse order) and also we looked at time for the `QueryMonitor.run()` loop to process expired queries. In every case, for 1e4 to 1e5 queries in the queue, the new implementation was faster.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
We don't really have any identifying information in our exception message regarding which query is actually being cancelled. In the original code run() loop, we logged the QueryThreadTask.toString(), which included the queryThread and queryCancelled. I'm not sure if that message was providing any value, but I presume if we log anything at all here it should be able to tell us _which_ query is being cancelled.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Change cancelled -> canceled
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Verify the formatting in the resulting javadoc.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Rename to queryMonitor.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "Bill (GitHub)" <gi...@apache.org>.
I think this thread pool constructor has to change so that its thread(s) is a `LoggingThread`. Should call the overload that takes a factory.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "Bill (GitHub)" <gi...@apache.org>.
Right @mcmellawatt. The point of the `DelayQueue` is that the item you want to remove is always at the head, reachable in O(1). From the doc:
> The head of the queue is that Delayed element whose delay expired furthest in the past.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #2625: GEODE-5568:
Increase wait in testCacheOpAfterQueryCancel()
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
This take could slow down a bit since it will now crawl the entire queue until it finds one that is expired. I don't know if this will actually matter overall.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Kind of oddly worded comment here
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Maybe phrase this as a non-question
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Extract 100 into static final field.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2625:
GEODE-5568: Increase wait in testCacheOpAfterQueryCancel()
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Should we assert that this exception is actually thrown? Maybe we can run a coverage tool and see if there are any gaps.
[ Full content available at: https://github.com/apache/geode/pull/2625 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org