[
http://jira.jboss.com/jira/browse/JGRP-736?page=comments#action_12409983 ]
Michael Newcomb commented on JGRP-736:
--------------------------------------
I don't understand your second argument, can you elaborate ?
If you get into the 'else' block, it is because there was only 1 request on the
queue. So the else block waits max_bundling_time and checks the queue to see if it should
keep going. The problem is that if there is now another request on the queue, it just
keeps going and removes the request from the queue and adds it to the requests list
without checking if the newly removed request can be processed with the request(s) on the
requests list. It only checks to see if multiple requests can be processed together if
there is another request on the queue immediately after a remove call.
Another flaw...
Yes. I discovered this one during testing. I fixed it by doing:
if (wait_time_ > 0)
{
Util.sleep(wait_time).
Request nextReq = (Request) q.peek();
keepGoing = view_bundling && firstRequest.canBeProcessedTogether(nextReq);
}
else
{
keepGoing = false;
}
we also cannot use sleep() to wait out the timeout, but have to get
notified when other requests are added
Right. I think to get this behavior, just modify the code above to do:
if (wait_time_ > 0)
{
Request nextReq = (Request) q.peek(wait_time);
keepGoing = view_bundling && firstRequest.canBeProcessedTogether(nextReq);
}
else
{
keepGoing = false;
}
Now, peek() will return when something is put on the queue and end the waiting.
GMS.ViewHandler.run() view bundling not correctly checking if
requests can be processed together...
---------------------------------------------------------------------------------------------------
Key: JGRP-736
URL:
http://jira.jboss.com/jira/browse/JGRP-736
Project: JGroups
Issue Type: Bug
Reporter: Michael Newcomb
Assigned To: Bela Ban
Fix For: 2.6.3, 2.7
Consider:
1356 do {
1357 Request firstRequest=(Request)q.remove(INTERVAL); // throws
a TimeoutException if it runs into timeout
1358 requests.add(firstRequest);
1359 if(q.size() > 0) {
1360 Request nextReq=(Request)q.peek();
1361 keepGoing=view_bundling &&
firstRequest.canBeProcessedTogether(nextReq);
1362 }
1363 else {
1364 stop=System.currentTimeMillis();
1365 wait_time=max_bundling_time - (stop-start);
1366 if(wait_time > 0)
1367 Util.sleep(wait_time);
1368 keepGoing=q.size() > 0;
1369 }
1370 }
1371 while(keepGoing);
The faulty logic occurs on line 1368. At this point there was only 1 object on the queue
when it was removed, so it waited max_bundling_time. After the wait_time expires, it
checks to see if q.size() > 0, if so, it keeps going.
This actually creates 2 problems.
1. if view_bundling is false, it will still bundle the next request because it goes right
back up and removes it and adds it to requests
2. if the request in the queue can't be processed with the request that just came in,
it will still be added to requests
So, the simple solution (for released versions) is to use the checks from 1360-1361:
1367.5 Request nextReq=(Request)q.peek();
1368 keepGoing=view_bundling && ((Request)
q.getLast()).canBeProcessedTogether(nextReq);
I think the ViewHandler could be rewritten for later releases so that as requests come
in, it notifies the ViewHandler thread and he can check all queued requests to see if they
could be processed together. Therefore, if you had view_bundling enabled, but a request
comes in that can't be processed with existing requests, it will start processing
immediately instead of waiting the rest of max_bundling_time (not long, but could be if
configured that way).
Also, if view_bundling is disabled, it should not sleep at all.
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://jira.jboss.com/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
http://www.atlassian.com/software/jira