[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689117698 I pushed some fixes: - Removed the AtomicInteger for the port number. As the server is running in test, we just call the server directly and it uses a Consumer(InetAddr) as callback. - Because of the callback, we can start the child processes without using a thread(pool). - Cleanup of processes improved. Keep in mind: The waitFor of 15s is is now way too much. Once the server ended and we run into the cleanup, all processes dies already, because the server shuts down when all clients dicsonnected. The cleanup is only needed if anything goes wrong on startup (like client can't connect to server due to network error on lo interface). 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689352137 The import error was fixed already. It was not caused by this PR, but a change in master. 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689369526 I merged from master to retrigger the check. Clicking on "Rerun checks" did not work, as it merged into same master commit as before. Makes no sense to me (maybe because of reproducibility). 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689410718 > I don't want to block the changes here, but just think we should make a followup to implement the same test, but with clients in the same JVM. > > There are two separate cases for NIO locks (held by same VM vs held by another process), so we should stress test them both. > Past experience has shown that whatever lock testing we have, it is somehow never enough. I think this could be a separate issue. Basically, this just "restores" the test in master, which was done in Ant only. There are no functional changes. But this new code is easy to adapt, there are several possibilities: - We can clone the whole test and just replaces `List processes` by a thread pool and instead of spawning a JVM we just call main() in a thread. Cleanup is more or less the same. - We can modify the current test to not only spawn multiple JVMs, but aditionally also add a thread. We can even randomize the number of clients/threads (currently fixed to 2 clients, but it's just a change in test config). In both cases, the policy also needs the "connect" permission. An alternative would be to allow client/server to alternatively communicate through a PipedInput/OutputStream (if its in same JVM). Some questions before I commit this: - Should I rename the test to TestStressLockFactories? 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689463433 Hi @dweiss indeed this looks now very complicated (before the test was so simple after my changes from last night). Indeed I want to remove some abstraction, the factory is unneeded. I always try to use the default functional interfaces provided by Java 8, and just creating one for exactly one methods is code bloat. A simple if/else in the factory method would be as fine. We should maybe also change the number of client to 3 instead of 2. Also during nightly tests we can hammer with even more clients. What I committed in: I disabled all custom Mock Filesystems, as this is important to run on native OS without any wrapping. 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689467186 I'd also change the test name now to `TestStressLockFactories`. 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689479947 Hi, I reordered the methods a bit in the class and applied some changes with number of clients: - Default is now 3. Actually this does not change the test runtime (on my machine) - On nightly we create 8 clients I would still like to get rid of the Supplier. IMHO, we should remove one randomization level. As we now have 3 clients (or 8), a randomization could always have a 2:1 chance to produce a forked client. What do you think, @dweiss ? 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689480950 FYI, the limit to 2 clients was caused by ANT's xml. Every client used another XML copypaste. But actually the number of clients is not really a performance factor. 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689546656 Hi, I refactored the code further and removed the randomization: - If uses `(clientId % 4)==0` to produce a thread client and otherwise a process client - In default mode, it creates 3 clients, so one of them is always in-process - In nightly mode, it creates 8 clients, so two of them are in-process I think that covers all modes. I think it's ready to commit. Question: Also backport to 8.x and remove the additional ANT task? 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689562340 Hi again, after reading the code of the LockStressTest: It tries to re-obtain a lock in the same JVM based on randomization. So actually the original test did everything right. @rmuir Can you verify this? So IMHO, we can remove the inprocess stuff and rever back to the state from this morning. 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842:
URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689563876
Code is here:
```java
try (final Lock l = verifyLF.obtainLock(lockDir, LOCK_FILE_NAME)) {
if (rnd.nextInt(10) == 0) {
if (rnd.nextBoolean()) {
verifyLF = new
VerifyingLockFactory(getNewLockFactory(lockFactoryClassName), in, out);
}
try (final Lock secondLock = verifyLF.obtainLock(lockDir,
LOCK_FILE_NAME)) {
throw new IOException("Double obtain");
} catch (LockObtainFailedException loe) {
// pass
}
}
Thread.sleep(sleepTimeMS);
} catch (LockObtainFailedException loe) {
// obtain failed
}
```
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689574638 I was talking with Robert. Let's revert the in-process stuff. Makes not much difference. We actually have a very good test for in-process logs, part of BaseLockFactoryTestCase. 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689584918 OK, reverted most of the changes in the stress tester. I kept the changes done by @dweiss about the magic numbers like starting gun "43" or access/release in the wire protocol. 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689649444 @dweiss Are you fine with this? I would like to commit and backport this soon. Unless you have some minor comments about the cleanup process, as I had to remove your abstraction on top of that. 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on pull request #1842: LUCENE-9512: Move LockFactory stress test to be a unit/integration test
uschindler commented on pull request #1842: URL: https://github.com/apache/lucene-solr/pull/1842#issuecomment-689700778 I backported this also to 8.x, removing the Ant targets related to this. Much cleaner now! 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: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
