New facility to obtain data for PageList's

Jay Shaughnessy jshaughn at redhat.com
Wed Sep 4 13:32:48 UTC 2013


On 9/4/2013 9:12 AM, Lukas Krejci wrote:
> Hi,
>
> tl;dr
> * use QueryUtility.fetchPagedDataAndCount() in SLSB methods returning a
> PageList,
> * use PageList.isConsistent() or PageControl.isConsistentWith() to detect if
> the page of the results shown has some potential problems.
>
> Now the long version ;)
>
> With the fix for https://bugzilla.redhat.com/show_bug.cgi?id=855674, there's a
> new facility in the server/jar module to query for paged data and total count.
> This work was done as part of the investigation of the "PageList was passed an
> empty collection but 'totalSize' was X" exception that we suffered from for a
> long time.
>
> Barring other programming mistakes, one of the most common reasons for the
> above exception (and one that was out of our control) was the "phantom read"
> phenomenon caused by our choice of transaction isolation level.

To be fair, I don't think we really had a choice other than 
read_committed.  I don't think we actually brought this upon ourselves.

>
> The phenomenon causes 2 queries for the same data within 1 transaction to have
> the possibility of returning different data (because of some other transaction
> committing changes in between the queries' executions). This in turn could
> have resulted in the above exception even if our code did everything right
> (which it of course does ;) ).
>
> The phenomenon is impossible to circumvent completely without upping the
> transaction isolation level to "serialized", which has severe performance
> implications (and new types of errors to deal with).
>
> We're most vulnerable to the phenomenon when we're returning paged data from
> our SLSB layer, which we do a lot. This is because we first query for the data
> and then perform a similar query to get the total count of the rows. We return
> this info in PageList objects which together with the PageControl objects
> implement our support for paging the data sets.
>
> The fix for the BZ contains a facility to alleviate the problems caused by
> phantom reads (i.e. inconsistent results returned from the data and count
> queries) - a new method:
>
> org.rhq.enterprise.server.util.QueryUtility.fetchPagedDataAndCount()
>
> This method is used when performing our criteria queries but is NOT used by
> the rest of our SLSB methods that still compose the PageList results on their
> own.
>
> I suggest we start replacing such occurences with the new method so that we
> gain the benefits of fewer phantom reads in other parts of the code base, not
> just criteria queries.
>
> The function of the method is fairly simple:
> it detects if the data and count obtained from the queries are consistent with
> the provided page control object (by using a new
> PageControl.isConsistentWith() method) and retries the queries a couple of
> times (waiting between retries for an increasing amount of time) to see if the
> phantom read situation "clears" itself. The number of retries as well as min
> and max wait times are configurable via system properties. For the vast
> majority of times, this method does not impose any additional performance
> cost, because phantom reads are fairly rare.
>
> Let me mention another aspect of the fix. The exception "PageList was passed
> an empty collection but 'totalSize' was X" was thrown in that very specific
> condition - if a PageList constructor was passed an empty collection and
> totalSize > 0.
>
> This behavior has been changed. This comes from realization that most of the
> time the above condition is either not an error at all or is caused by
> circumstances outside of influence of either user or the codebase (phantom
> reads, heavy concurrent activity on the database, etc).
>
> The exception could be caused by 2 things (outside of a programming error,
> which is indistinguishable from the 2 things in that constructor):
> 1) Trying to read a page past the total number of pages in a result,
> 2) a phantom read
>
> I do believe that none of them should be considered severe enough a condition
> to throw an exception and abort the callchain abruptly. Trying to read past
> the number of available pages can easily occur when 1 user looks at the
> second-last page of some results that other user remove some items from. When
> the first user is ready to view the last page (as he understands it), the data
> is no longer there. Is that an error that should be shown to the user or
> should we just return empty result set? I personally think the latter is more
> user-friendly.

I agree.

>
> While we can try to avoid returning inconsistent data from a phantom read,
> were cannot completely avoid that from happening and again, I don't think we
> should abruptly throw an exception and let the user stare at the screen in
> disbelief that a mere paging through results can sometimes result in errors.
>
> In both cases, I think a much better approach is to detect such occurences and
> possibly warn the user that such thing might have happened. This is now doable
> using PageList.isConsistent() and equivalent PageControl.isConsistentWith()
> methods that can check whether given data, the declared total size and the
> provided page control are consistent with each other. The CLI users can now
> take advantage of those methods, but we still need to add support to the Table
> class (or wherever it would be) in GUI to show some warning strips or whatever
> to the user.

So what would good handling look like?  Is there a recommended best 
practice for when things are no longer consistent?  A refresh of the 
view, or something like that?

>
> Note that such consistency has much more elaborate rules than the "empty
> collection, total size > 0". I suggest you read the javadoc and source of
> PageControl.isConsistentWith(Collection, int) method for the full discussion
> on which states are considered consistent and which aren't.
>
> In any case, I'd like to ask you read through the new code implemented in
> commits dc6a1fedea3bfa9efdfde446d1712d353a591778 and
> e82330bd6af45fe519f2eb85e81ca02a97b1eb04 (the latter being a reimplementation
> of the first to make the method reusable outside of criteria queries) and see
> if you spot any errors or places that could be improved.
>
> Thanks,
>
> Lukas
>
>
>
>
> _______________________________________________
> rhq-devel mailing list
> rhq-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/rhq-devel



More information about the rhq-devel mailing list