You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by xiaojian zhou <zh...@gmail.com> on 2016/05/05 06:23:05 UTC
Review Request 47004: a quick solution to wait for entry flushed into
index
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47004/
-----------------------------------------------------------
Review request for geode and Dan Smith.
Bugs: geode-1351
https://issues.apache.org/jira/browse/geode-1351
Repository: geode
Description
-------
This is for test purpose.
An advanced version will be implemented later using function execution.
Diffs
-----
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImpl.java 58a9b20
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java c467a18
Diff: https://reviews.apache.org/r/47004/diff/
Testing
-------
Thanks,
xiaojian zhou
Re: Review Request 47004: a quick solution to wait for entry flushed
into index
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47004/#review131921
-----------------------------------------------------------
It seems like maybe the Flush API ought to be on the index, not the LuceneServiceImpl. And I thought the intention was to add this to the public API, but it looks like right now this is just an internal API?
Please add true unit test for these changes.
Regarding your integration test, I don't think this is the right way to test this. You're just assuming the entries won't be flushed by the time your waitForFlush call happens, but what's to prevent them from being flushed by that point? Also, the integration tests should run as fast as possible, so an integration test that puts 1K entries seems excessive.
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImpl.java (line 243)
<https://reviews.apache.org/r/47004/#comment195962>
Remove this log message - we don't need an info level log here.
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java (line 81)
<https://reviews.apache.org/r/47004/#comment195963>
Maybe name this something like
entriesFlushedToIndexAfterWaitForFlushCalled?
- Dan Smith
On May 5, 2016, 6:23 a.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47004/
> -----------------------------------------------------------
>
> (Updated May 5, 2016, 6:23 a.m.)
>
>
> Review request for geode and Dan Smith.
>
>
> Bugs: geode-1351
> https://issues.apache.org/jira/browse/geode-1351
>
>
> Repository: geode
>
>
> Description
> -------
>
> This is for test purpose.
>
> An advanced version will be implemented later using function execution.
>
>
> Diffs
> -----
>
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImpl.java 58a9b20
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java c467a18
>
> Diff: https://reviews.apache.org/r/47004/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 47004: a quick solution to wait for entry flushed
into index
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47004/#review133395
-----------------------------------------------------------
We have some other tests that had work arounds for this issue. Perhaps we can fix these to use the new flush method? For example, I think LuceneIndexCreationIntegrationTest we added Awaitility.await().atMost(60, TimeUnit.SECONDS).until(() -> {
assertEquals(Arrays.asList("field1"), field1Analyzer.analyzedfields);
assertEquals(Arrays.asList("field2"), field2Analyzer.analyzedfields);
});
Or in LuceneIndexRecoveryHAIntegrationTest
private void waitUntilQueueEmpty(final String aeqId) {
// TODO flush queue
AsyncEventQueue queue = cache.getAsyncEventQueue(aeqId);
Awaitility.waitAtMost(1000, TimeUnit.MILLISECONDS).until(() -> assertEquals(0, queue.size()));
}
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java (line 103)
<https://reviews.apache.org/r/47004/#comment197790>
Should we throw an exception if we never get to 0?
- Jason Huynh
On May 16, 2016, 5:39 a.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47004/
> -----------------------------------------------------------
>
> (Updated May 16, 2016, 5:39 a.m.)
>
>
> Review request for geode, anilkumar gingade and Dan Smith.
>
>
> Bugs: geode-1351
> https://issues.apache.org/jira/browse/geode-1351
>
>
> Repository: geode
>
>
> Description
> -------
>
> This is for test purpose.
>
> An advanced version will be implemented later using function execution.
>
>
> Diffs
> -----
>
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneEventListener.java 9fdfd43
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java 0b5f8fa
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java c467a18
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImplJUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/47004/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 47004: a quick solution to wait for entry flushed
into index
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47004/#review133393
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java (line 86)
<https://reviews.apache.org/r/47004/#comment197784>
Do we need to say about advanced version?
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java (line 88)
<https://reviews.apache.org/r/47004/#comment197776>
We need to define this in LunceneIndex interface to be used as public API.
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java (line 89)
<https://reviews.apache.org/r/47004/#comment197779>
I believe in our group session we passed two arguments. 1. Max wait time 1. Unit of sleep (iunstead of hard coded 200 mili sec)
- anilkumar gingade
On May 16, 2016, 5:39 a.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47004/
> -----------------------------------------------------------
>
> (Updated May 16, 2016, 5:39 a.m.)
>
>
> Review request for geode, anilkumar gingade and Dan Smith.
>
>
> Bugs: geode-1351
> https://issues.apache.org/jira/browse/geode-1351
>
>
> Repository: geode
>
>
> Description
> -------
>
> This is for test purpose.
>
> An advanced version will be implemented later using function execution.
>
>
> Diffs
> -----
>
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneEventListener.java 9fdfd43
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java 0b5f8fa
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java c467a18
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImplJUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/47004/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 47004: a quick solution to wait for entry flushed
into index
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47004/#review133405
-----------------------------------------------------------
I think you misunderstood my review comment... I was wondering if you could change the tests to use your new method and remove the old awaitility work arounds...
Also, I was wondering if we should throw an exception if the flush method exceeds the max wait time. Then the test will know exactly that the queue did not flush properly...
- Jason Huynh
On May 16, 2016, 5:39 a.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47004/
> -----------------------------------------------------------
>
> (Updated May 16, 2016, 5:39 a.m.)
>
>
> Review request for geode, anilkumar gingade and Dan Smith.
>
>
> Bugs: geode-1351
> https://issues.apache.org/jira/browse/geode-1351
>
>
> Repository: geode
>
>
> Description
> -------
>
> This is for test purpose.
>
> An advanced version will be implemented later using function execution.
>
>
> Diffs
> -----
>
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneEventListener.java 9fdfd43
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java 0b5f8fa
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java c467a18
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImplJUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/47004/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 47004: a quick solution to wait for entry flushed
into index
Posted by xiaojian zhou <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47004/
-----------------------------------------------------------
(Updated May 16, 2016, 9:58 p.m.)
Review request for geode, anilkumar gingade and Dan Smith.
Changes
-------
let it return a boolean and use it in a few test cases
Bugs: geode-1351
https://issues.apache.org/jira/browse/geode-1351
Repository: geode
Description
-------
This is for test purpose.
An advanced version will be implemented later using function execution.
Diffs (updated)
-----
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/LuceneIndex.java be329f7
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java 981d9e4
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/xml/LuceneIndexCreation.java b54f51b
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexCreationIntegrationTest.java 6429143
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexRecoveryHAIntegrationTest.java 77d2a5c
Diff: https://reviews.apache.org/r/47004/diff/
Testing
-------
Thanks,
xiaojian zhou
Re: Review Request 47004: a quick solution to wait for entry flushed
into index
Posted by xiaojian zhou <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47004/
-----------------------------------------------------------
(Updated May 16, 2016, 5:39 a.m.)
Review request for geode, anilkumar gingade and Dan Smith.
Changes
-------
apply reviewer's comments, added junit test and dunit tests
Bugs: geode-1351
https://issues.apache.org/jira/browse/geode-1351
Repository: geode
Description
-------
This is for test purpose.
An advanced version will be implemented later using function execution.
Diffs (updated)
-----
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneEventListener.java 9fdfd43
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java 0b5f8fa
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java c467a18
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImplJUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/47004/diff/
Testing
-------
Thanks,
xiaojian zhou