[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-677581744 Hi @jujoramos , I wish that I spotted it earlier so you don't waste your time on this. Thanks again for a help! New [PR](https://github.com/apache/geode/pull/5466) is opened. 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
[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-677527803 > I'm still not sure that the changes within the `DiskStoreCommandUtils` class are needed to address the problem. I've opened a draft [PR](https://github.com/apache/geode/pull/5463) with your test and made a simple change within the `DiskStoreImpl` class, which seems to address this issue more easily with less impact (see [here](https://github.com/apache/geode/pull/5463/files#diff-05e3599f18f88368a85d5fbd987a1094L463)). Can you have a look and let me know your thoughts?. > I've also added some extra reviewers as they have some more knowledge around the `disk-store` area and the related `gfsh` commands. Hi @jujoramos , I agree with you and your proposal, as it will include less changes with less impact. I tested your changes and will create a new PR with your proposal and tests. Thank you for a help! 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
[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-674786552 > Hello @mkevo , > > I've manually copied and executed the test `OfflineDiskStoreCommandsDUnitTest.testThreadHangWithOfflineDiskStoreCommands()` locally on the current `develop` branch (doesn't contain your fix for `GEODE-8119`) and it passes just fine, so it's not actually exhibiting the bug you intend to fix. > As a said [before](https://github.com/apache/geode/pull/5175#issuecomment-638793177), I'm hesitant to approve the changes until there's at least one test showing that the original bug exists, there's no other way to verify that the actual fix works as intended. Once that's done, we can discuss whether the changes to `DiskStoreCommandsUtils` should be included or not. Hi @jujoramos , Please verify now, it seems that I delete writer.close by mistake so the file was empty. 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
[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-671872424 Hi @jujoramos, After adding some logs I saw that in `offlineCompact` method it not executes `dsi.close() ` as it fails before on `createForOffline` while loading files in DiskStoreImpl. So addding this close will not help to remove this thread as it never comes to it if wrong name is provided, but closing is called from `cleanupOffline()` in finally block. The same thing is for `offlineCompact`. Correct me if I'm wrong. 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
[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-671311587 > Sure, can you point me exactly to which commands you're referring here?. The original ticket ([GEODE-8119](https://issues.apache.org/jira/browse/GEODE-8119)) doesn't mention which commands are actually hitting the bug itself. I'm referring to **describe offline-disk-store** and **alter disk-store** commands. 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
[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-671299214 > @mkevo > Thanks for adding the extra tests. I'm not fully convinced about the implemented fix, however, did you consider my comments [here](https://github.com/apache/geode/pull/5175#issuecomment-638793177)?. Can't you just simply invoke the `DiskStoreImpl.close()` method instead of modifying the `DiskStoreCommandsUtils` class, as it involves way less modifications and we're sure it won't affect other areas of the `disk` management stuff?. I would appreciate if you can specify how to invoke this command as there is no method like `offlineCompact` in _DiskStoreImpl.java_ for describe command and others. The only change I made on `DiskStoreCommandsUtils` class that it also check name, not only dirs. As if name is wrong it will start thread and then fail without closing. Thanks! 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
[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-657509799 > Hello @mkevo, > > This PR has been inactive for quite some time now, should we close it or are you planning to continue working on it?. Hi @jujoramos, I have some other commitments, as soon as possible I will come back to this. 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
[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-638049210 @jujoramos In all offline disk-store commands it calls DiskStoreImpl in which is startAsyncFlusher which starts these threads. E.g. _AlterOfflineDiskStoreCommand_ use _modifyRegion_ which have _createForOffline_ method in which _DiskStoreImpl_ constructor is called where it start this thread with _startAsyncFlusher_. But if set that it validate also name at the start, not only dirs it will not be started. Please see my latest changes and verify it with your tests. 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
[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-637544676 > @mkevo > One thing I forgot to mention is that I ran the internal tests again and I can see exactly the same failure messages as before: > > ``` > Executing - validate offline-disk-store --disk-dirs=/var/vcap/data/rundir/concUpgradeVVXHV/concUpgrade-0602-045131/vm_7_oldVersion2_disk_4,/var/vcap/data/rundir/concUpgradeVVXHV/concUpgrade-0602-045131/vm_7_oldVersion2_disk_3,/var/vcap/data/rundir/concUpgradeVVXHV/concUpgrade-0602-045131/vm_7_oldVersion2_disk_2,/var/vcap/data/rundir/concUpgradeVVXHV/concUpgrade-0602-045131/vm_7_oldVersion2_disk_1 --name=diskStore1 > > Standard error was: > Could not find: "/BACKUPdiskStore1.if/BACKUPdiskStore1.if/BACKUPdiskStore1.if" > ``` > > Also, and just for your reference, keep in mind that the `.if` file _**is only stored**_ in the first `disk-dir` listed for the store itself, not for everyone `disk-dir` configured for that particular `disk-store` (see [here](https://geode.apache.org/docs/guide/112/managing/disk_storage/file_names_and_extensions.html)). I reproduced the issue you have. It is for the wrong disk-store name. I will add test for that also and fix it. Before this changes it failed with java.lang.IllegalStateException when wrong name is provided, but the thread is already started and not closed. So I think that I need to do change on `DiskStoreCommandsUtils` as thread will also hang if the wrong name is added as there is no name validation before starting it, as it is for dirs. 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
[GitHub] [geode] mkevo commented on pull request #5175: GEODE-8119: Threads hangs when offline disk store command is invoked
mkevo commented on pull request #5175: URL: https://github.com/apache/geode/pull/5175#issuecomment-635901434 > Hey @mkevo > Thanks for working on this, I'll start the review once all `CI` tests are green. > Cheers. It looks that last five opened PRs failed on LGTM check. ``` [2020-05-29 00:08:56] [analysis] [EVALUATION 115/177] [FAIL] Error running query semmlecode-queries/Security/CWE/CWE-022/TaintedPath.ql: OutOfMemory Query evaluation ran out of memory (maximum allowed memory: 3012MB). ``` 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
