Quantcast

[GitHub] activemq-artemis pull request #1007: ARTEMIS-958 Improve web server tmp dir ...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1007: ARTEMIS-958 Improve web server tmp dir ...

clebertsuconic-3
GitHub user gaohoward opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1007

    ARTEMIS-958 Improve web server tmp dir cleanup

    When server is shutdown by user the shutdown hook will check if the tmpdir of the web server is cleaned up. However the cleanup is also performed in a shutdown hook (using File.deleteOnExit). Because the order of execution of hooks is not guaranteed, if the tmp dir is not cleaned up by the time of check, we should add a 'force' delete to make sure the tmp dir is removed after server stop.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/gaohoward/activemq-artemis master-tmpdir

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1007.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1007
   
----
commit 340c0b3e8c106554db6631950ee1ed5c2ecb5f45
Author: Howard Gao <[hidden email]>
Date:   2017-02-13T03:45:41Z

    ARTEMIS-958 Improve web server tmp dir cleanup

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1007: ARTEMIS-958 Improve web server tmp dir cleanup

clebertsuconic-3
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1007
 
    The information on the pull request is to great to be lost. Why don't you amend the commit with it?  Or Put on the Jira.  



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1007: ARTEMIS-958 Improve web server tmp dir cleanup

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user gaohoward commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1007
 
    ok will do it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1007: ARTEMIS-958 Improve web server tmp dir ...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1007#discussion_r100815172
 
    --- Diff: artemis-web/src/main/java/org/apache/activemq/artemis/ActiveMQWebLogger.java ---
    @@ -57,4 +57,8 @@
        @LogMessage(level = Logger.Level.WARN)
        @Message(id = 244003, value = "Temporary file not deleted on shutdown: {0}", format = Message.Format.MESSAGE_FORMAT)
        void tmpFileNotDeleted(File tmpdir);
    +
    +   @LogMessage(level = Logger.Level.DEBUG)
    --- End diff --
   
    We don't need to use the Logger for Debug Messages...
   
   
    just make these calls
   
   
   
    on the declarion of the class: private static final Logger logger = Logger.getLogger(...); // use the right package here.. and there are other examples you can follow...
   
   
   
    and then log.debug(".... .static text...")
   
   
   
    These messages are for INFO, WARN, ERROR.. or things that will need a code to be printed out.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1007: ARTEMIS-958 Improve web server tmp dir ...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1007#discussion_r100815323
 
    --- Diff: artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java ---
    @@ -145,9 +146,16 @@ public void internalStop() throws Exception {
                    //tmpdir will be removed by deleteOnExit()
                    //somehow when broker is stopped and restarted quickly
                    //this tmpdir won't get deleted sometimes
    -               boolean fileDeleted = TimeUtils.waitOnBoolean(false, 10000, tmpdir::exists);
    +               boolean fileDeleted = TimeUtils.waitOnBoolean(false, 5000, tmpdir::exists);
                    if (!fileDeleted) {
    -                  ActiveMQWebLogger.LOGGER.tmpFileNotDeleted(tmpdir);
    +                  //because the execution order of shutdown hooks are
    +                  //not determined, so it's possible that the deleteOnExit
    +                  //is executed after this hook, in that case we force a delete.
    +                  FileUtil.deleteDirectory(tmpdir);
    +                  ActiveMQWebLogger.LOGGER.tmpFileForceDelete(tmpdir);
    --- End diff --
   
    ActiveMQWebLogger.LOGGER.debug("..."); would also work... but I prefer to have the actual logger on debug or trace


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1007: ARTEMIS-958 Improve web server tmp dir ...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user gaohoward commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1007#discussion_r100942014
 
    --- Diff: artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java ---
    @@ -145,9 +146,16 @@ public void internalStop() throws Exception {
                    //tmpdir will be removed by deleteOnExit()
                    //somehow when broker is stopped and restarted quickly
                    //this tmpdir won't get deleted sometimes
    -               boolean fileDeleted = TimeUtils.waitOnBoolean(false, 10000, tmpdir::exists);
    +               boolean fileDeleted = TimeUtils.waitOnBoolean(false, 5000, tmpdir::exists);
                    if (!fileDeleted) {
    -                  ActiveMQWebLogger.LOGGER.tmpFileNotDeleted(tmpdir);
    +                  //because the execution order of shutdown hooks are
    +                  //not determined, so it's possible that the deleteOnExit
    +                  //is executed after this hook, in that case we force a delete.
    +                  FileUtil.deleteDirectory(tmpdir);
    +                  ActiveMQWebLogger.LOGGER.tmpFileForceDelete(tmpdir);
    --- End diff --
   
    I see. will do that. Thanks



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1007: ARTEMIS-958 Improve web server tmp dir ...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1007#discussion_r101185508
 
    --- Diff: artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java ---
    @@ -145,9 +149,16 @@ public void internalStop() throws Exception {
                    //tmpdir will be removed by deleteOnExit()
                    //somehow when broker is stopped and restarted quickly
                    //this tmpdir won't get deleted sometimes
    -               boolean fileDeleted = TimeUtils.waitOnBoolean(false, 10000, tmpdir::exists);
    +               boolean fileDeleted = TimeUtils.waitOnBoolean(false, 5000, tmpdir::exists);
                    if (!fileDeleted) {
    -                  ActiveMQWebLogger.LOGGER.tmpFileNotDeleted(tmpdir);
    +                  //because the execution order of shutdown hooks are
    +                  //not determined, so it's possible that the deleteOnExit
    +                  //is executed after this hook, in that case we force a delete.
    +                  FileUtil.deleteDirectory(tmpdir);
    +                  logger.debug("Force to delete temporary file on shutdown: " + tmpdir.getAbsolutePath());
    --- End diff --
   
    I'm going to merge this,
    but just as a future reference, we do a lot of if (logger.isDebugEnabled());
   
    This is a negative test and it won't be affected, so no big deal, and it will be merged..
   
   
    just saying that for future reference.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1007: ARTEMIS-958 Improve web server tmp dir ...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user asfgit closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/1007


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1007: ARTEMIS-958 Improve web server tmp dir ...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user gaohoward commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1007#discussion_r101186821
 
    --- Diff: artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java ---
    @@ -145,9 +149,16 @@ public void internalStop() throws Exception {
                    //tmpdir will be removed by deleteOnExit()
                    //somehow when broker is stopped and restarted quickly
                    //this tmpdir won't get deleted sometimes
    -               boolean fileDeleted = TimeUtils.waitOnBoolean(false, 10000, tmpdir::exists);
    +               boolean fileDeleted = TimeUtils.waitOnBoolean(false, 5000, tmpdir::exists);
                    if (!fileDeleted) {
    -                  ActiveMQWebLogger.LOGGER.tmpFileNotDeleted(tmpdir);
    +                  //because the execution order of shutdown hooks are
    +                  //not determined, so it's possible that the deleteOnExit
    +                  //is executed after this hook, in that case we force a delete.
    +                  FileUtil.deleteDirectory(tmpdir);
    +                  logger.debug("Force to delete temporary file on shutdown: " + tmpdir.getAbsolutePath());
    --- End diff --
   
    Got it, thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Loading...