[GitHub] wicket pull request #212: WICKET-6177 Blocking page serialization

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] wicket pull request #212: WICKET-6177 Blocking page serialization

bitstorm
GitHub user manuelbarzi opened a pull request:

    https://github.com/apache/wicket/pull/212

    WICKET-6177 Blocking page serialization

    from https://issues.apache.org/jira/browse/WICKET-6177
   
    **AsyncPageStore** adds the capability to provide an asynchronous page storing by wrapping the original sync page store (e.g. DefaultPageStore) and handling store requests in a queue and storing pages in worker thread which makes use of the original store. The number of pages to be managed asynchronously is defined by a capacity, provided by estimation which should be done on each specific application where applied.
   
    The aim of AsyncPageStore is to unblock page requests when serialization process takes 'too long', giving the user the sensation of 'slow web'. This could happen in applications that for any reason delay on backing up pages by means of the current default wicket store.
   
    It adds the chance to manage a preset number of pages asynchronously, defined by the 'capacity', and in case that limit is exceeded by requests, it just works as the original sync page store, by simply delegating work on it until the queue is released and space is done to handle pages in asynchronous way again.
   
    It acts in a way like saying "let's handle storing of pages asynchronously for an estimated number of them in 'normal conditions', and in case there is an excess of pages to store (peak of requests), that is, exceeding async store 'capacity', then let's just work as normally.
   
    **AsyncPageStoreTest** provides initial on/off cases with the scenarios in which async page store works in 'normal conditions' (not exceeding capacity), and the opposite (arriving at a point of exceeding that capacity).
   
    original code is already 'gutted' in a **quickstart** for inspection and testing in https://github.com/manuelbarzi/wicket-async-page-store
   
    for further details on this projection, just following the first link above.

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

    $ git pull https://github.com/manuelbarzi/wicket master

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

    https://github.com/apache/wicket/pull/212.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 #212
   
----
commit 2ffd587dea4c5e1edec0853f35a9b5761e391917
Author: manuelbarzi <[hidden email]>
Date:   2017-02-12T01:23:40Z

    WICKET-6177 Blocking page serialization

----


---
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
|

[GitHub] wicket issue #212: WICKET-6177 Blocking page serialization

bitstorm
Github user bitstorm commented on the issue:

    https://github.com/apache/wicket/pull/212
 
    Hi @manuelbarzi
    thank you for you PR. I see you added Guava as dependency but I don't see any point where you use it. Am I missing something?


---
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
|

[GitHub] wicket issue #212: WICKET-6177 Blocking page serialization

bitstorm
In reply to this post by bitstorm
Github user manuelbarzi commented on the issue:

    https://github.com/apache/wicket/pull/212
 
    ciao @bitstorm
   
    yes, indeed, guava and commons-lang3, both for test purposes - test scope / pom - in AsyncPageStoreTest, making use of com.google.common.base.Stopwatch and org.apache.commons.lang3.RandomUtils, respectively.


---
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
|

[GitHub] wicket issue #212: WICKET-6177 Blocking page serialization

bitstorm
In reply to this post by bitstorm
Github user bitstorm commented on the issue:

    https://github.com/apache/wicket/pull/212
 
    ciao @manuelbarzi ,
   
    my fault, I'm not very familiar with Guava and its package naming.


---
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
|

[GitHub] wicket issue #212: WICKET-6177 Blocking page serialization

bitstorm
In reply to this post by bitstorm
Github user mmakundi commented on the issue:

    https://github.com/apache/wicket/pull/212
 
    Thanks @manuelbarzi and Some findings:
   
    1. I would remove the (non-Javadoc) and instead format those with start-prefix /** like true javadoc and remove extra empty lines in the javadoc
   
    2. All in all check for formatting and extra lines etc.
   
    3. In case entries.offer fails, entryMap.remove is called before pageStore.storePage. If we assume pageStore.storePage is slow, maybe it would be good idea to postpone remove so that a new requiest during pageStore.storePage can use the in-memory reference? Similar to what is happening in PageSavingRunnable.run? Also this will make a great re-usable method:
    storePageAndRemoveFromMap(...) {
             log.debug("Saving asynchronously: {}...", entry);
            pageStore.storePage(sessionId, page);
            entryMap.remove(getKey(entry));
    }
   
    4. Same as in (3.) to catch (InterruptedException e) { log.error(e.getMessage(), e);; storePageAndRemoveFromMap(...); }
   
    5. Please junit-test if any conflict occur if unbind(sessionId) is called while there are entries in the queue for that session.
   
    6. Please add tests also for borderline racing cases such as same instance is returned for new request arriving before storing is complete.
   
    7. I would rename AsyncPageStore -> AsynchronousPageStore for clarity and symmetry (symmetric naming with AsynchronousDataStore),
   
    8. Please propose a patch also into DefaultPageManagerProvider such that  new AsyncPageStore(super.newPageStore(dataStore), 100); would be the default pagestore for wicket when dataStore.canBeAsynchronous()==true. Similar way that AsynchronousDataStore is default.


---
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
|

[GitHub] wicket issue #212: WICKET-6177 Blocking page serialization

bitstorm
In reply to this post by bitstorm
Github user 1nside0ut commented on the issue:

    https://github.com/apache/wicket/pull/212
 
    hi @mmakundi
   
    "3. In case entries.offer fails, entryMap.remove is called before pageStore.storePage. If we assume pageStore.storePage is slow, maybe it would be good idea to postpone remove so that a new requiest during pageStore.storePage can use the in-memory reference? Similar to what is happening in PageSavingRunnable.run? Also this will make a great re-usable method:
    storePageAndRemoveFromMap(...) {
    log.debug("Saving asynchronously: {}...", entry);
    pageStore.storePage(sessionId, page);
    entryMap.remove(getKey(entry));
    }"
   
    entryMap is not supposed to be used as "backup mem", there's already one queue for that (entries). it's not by mistake that map-removal is located before synchronous store call for the same thread (not the worker). the reason is simple: if before try-to-queue a page fails, or it simply takes to long (offer), then that would mean app mem / process is in trouble, "no space / speed for that", so there's not point on keeping that ref attached to map (occupying) while synchronous storing ("slow" storing, as you say) if it has already been stated a fail on trying to manage it in queue (mem). using map as "a backup for when queuing fails" would just false the mechanism, imposing a mem kept at a non-optimal point for that.
    on the other hand, regarding the worker thread that "asynchronously" stores the page, the case is the opposite. every single page that has achieved to enter the queue before (that is, offer succeeded), has already got the privilege to being kept in mem while not being entirely stored, so being removed from map after that.
    queue protects itself from accepting or not new entries depending on env conditions, but outside it logic should act in coherence to 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
|

[GitHub] wicket pull request #212: WICKET-6177 Blocking page serialization

bitstorm
In reply to this post by bitstorm
Github user asfgit closed the pull request at:

    https://github.com/apache/wicket/pull/212


---
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.
---