7:33 a.m.
Looks good. A couple of implementation notes:
1. ReplicationListener constructor - getting components from the cache
TestingUtil.extractField() is brittle (since it relies on field name)
and could break with even the slightest refactoring. Better to use
TestingUtil.extractComponentRegistry() and
ComponentRegistry.getComponent() to get the component you want. Much
safer.
2. ReplicationListener constructor - updating components in the cache
Again rather than using TestingUtil.replaceField() - equally brittle -
try using componentRegistry.registerComponent() to register a new
component, and componentRegistry.rewire() if the cache is already
running to ensure the new component is re-injected everywhere it is
needed.
3. How does this behave with sync replication? The javadocs say it
doesn't make any sense, but it may be done in base test classes, such
that the test could have subclasses which use sync and async
replication. I'm guessing waitForReplication() will always return
immediately with sync replication?
Cheers,
Manik
PS: cc'ing jbosscache-dev - discussions like this should be on the
list, just in case anyone else is interested. :-)
On 29 May 2008, at 08:41, Mircea Markus wrote:
I've just migrated o.j.c.replicated.AsyncReplTest to the new
listener mechanism.
Can you please take a look and let me know what do you think.
Cheers,
Mircea
Brian Stansberry wrote:
> Try o.j.c.replicated.AsyncReplTest
>
> Mircea Markus wrote:
>> Sorry, wrong button :)
>> Can you point me to such a class? I'll continue refactoring on Th
>> and I'll start with one of these for convenience. The plan is to
>> remove (almost) all 'sleeps' from code.
>>
>> Mircea Markus wrote:
>>> Can you point me
>>>
>>> Brian Stansberry wrote:
>>>> Thanks. Are you planning to add one for the ordinary REPL_ASYNC
>>>> case; i.e. no replication queue? That's the one where I end up
>>>> adding Thread.sleep() the most -- waiting to ensure the async
>>>> message has arrived on the remote node before testing failover.
>>>>
>>>> Mircea Markus wrote:
>>>>> Hi,
>>>>>
>>>>> Take a look at
>>>>> org.jboss.cache.util.internals.EvictionController and
>>>>> org.jboss.cache.util.internals.ReplicationQueueNotifier.
>>>>> Right now they are only being used in
>>>>> StateTransferConcurrencyTest, I will change more tests to use
>>>>> them later on this week. Any feedback is welcomed!
>>>>> Cheers,
>>>>> Mircea
>>>>
>
--
Manik Surtani
Lead, JBoss Cache
manik(a)jboss.org
8:43 a.m.
New subject: new utility classes for removing Thread.sleeps
Manik Surtani wrote:
Looks good. A couple of implementation notes:
1. ReplicationListener constructor - getting components from the cache
TestingUtil.extractField() is brittle (since it relies on field name)
and could break with even the slightest refactoring. Better to use
TestingUtil.extractComponentRegistry() and
ComponentRegistry.getComponent() to get the component you want. Much
safer.
Fair point. It's not that I've even though about this nice solution :) ,
but in this scenario it just doesn't apply because rpcDispatcher in
RPCmanagerImpl is build using 'new', not through dependency injection
and it is not even registered in the component registry.
2. ReplicationListener constructor - updating components in the
cache
Again rather than using TestingUtil.replaceField() - equally brittle -
try using componentRegistry.registerComponent() to register a new
component, and componentRegistry.rewire() if the cache is already
running to ensure the new component is re-injected everywhere it is
needed.
same reason as bellow, the marshaller is not injected.
3. How does this behave with sync replication? The javadocs say it
doesn't make any sense, but it may be done in base test classes, such
that the test could have subclasses which use sync and async
replication. I'm guessing waitForReplication() will always return
immediately with sync replication?
yes, this is a valid scenario so I'll amend
the comment. Some other
words about using it in the scenarios you mentioned: I've tried
integrating it in PFER test suite, which has a base test for
async/sync/optimistic/pessimistic/invalidation and it's just to
complicated to write the expectations in all scenarios (e.g. if no
explicit tx pessimistic replication expects the command,but optimistic
replication expects a commit command). The code would had been to
complicated (i.e. expectations were very different from config to config
so I ended up keeping
the old sleep (for this test only) for the sake of readability. PFER are
some stable tests, so I don't reckon there's a high risk of encountering
a failure due to sleep - if this will be the case I'll take the tests
and write then for each individual configuration rather than having a
base class.
Cheers,
Manik
PS: cc'ing jbosscache-dev - discussions like this should be on the
list, just in case anyone else is interested. :-)
On 29 May 2008, at 08:41, Mircea Markus wrote:
> I've just migrated o.j.c.replicated.AsyncReplTest to the new
> listener mechanism.
> Can you please take a look and let me know what do you think.
>
> Cheers,
> Mircea
>
> Brian Stansberry wrote:
>> Try o.j.c.replicated.AsyncReplTest
>>
>> Mircea Markus wrote:
>>> Sorry, wrong button :)
>>> Can you point me to such a class? I'll continue refactoring on Th
>>> and I'll start with one of these for convenience. The plan is to
>>> remove (almost) all 'sleeps' from code.
>>>
>>> Mircea Markus wrote:
>>>> Can you point me
>>>>
>>>> Brian Stansberry wrote:
>>>>> Thanks. Are you planning to add one for the ordinary REPL_ASYNC
>>>>> case; i.e. no replication queue? That's the one where I end up
>>>>> adding Thread.sleep() the most -- waiting to ensure the async
>>>>> message has arrived on the remote node before testing failover.
>>>>>
>>>>> Mircea Markus wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Take a look at org.jboss.cache.util.internals.EvictionController
>>>>>> and org.jboss.cache.util.internals.ReplicationQueueNotifier.
>>>>>> Right now they are only being used in
>>>>>> StateTransferConcurrencyTest, I will change more tests to use
>>>>>> them later on this week. Any feedback is welcomed!
>>>>>> Cheers,
>>>>>> Mircea
>>>>>
>>
--
Manik Surtani
Lead, JBoss Cache
manik(a)jboss.org
9:10 a.m.
New subject: new utility classes for removing Thread.sleeps
On 29 May 2008, at 14:43, Mircea Markus wrote:
Manik Surtani wrote:
> Looks good. A couple of implementation notes:
>
> 1. ReplicationListener constructor - getting components from the
> cache
>
> TestingUtil.extractField() is brittle (since it relies on field
> name) and could break with even the slightest refactoring. Better
> to use TestingUtil.extractComponentRegistry() and
> ComponentRegistry.getComponent() to get the component you want.
> Much safer.
>
Fair point. It's not that I've even though about this nice
solution :) , but in this scenario it just doesn't apply because
rpcDispatcher in RPCmanagerImpl is build using 'new', not through
dependency injection and it is not even registered in the component
registry.
> 2. ReplicationListener constructor - updating components in the
> cache
>
> Again rather than using TestingUtil.replaceField() - equally
> brittle - try using componentRegistry.registerComponent() to
> register a new component, and componentRegistry.rewire() if the
> cache is already running to ensure the new component is re-injected
> everywhere it is needed.
>
same reason as bellow, the marshaller is not injected.
Surely these components can be added to the ComponentRegistry? We'd
need a RpcDispatcherFactory, with knowledge of how to build the
necessary RpcDispatcher, have that injected into the RpcManager. We'd
also need a ChannelFactory and register the Channel as a component,
and declare it as a dependency of our RpcDispatcher implementation.
Waitaminit, this won't work. JGroups' RpcDispatcher, which we extend,
doesn't expose a no-arg constructor. Damn. Bela, Vladimir: any
chance of introducing a no-arg constructor for the RpcDispatcher?
Surely others who have tried to use the RpcDispatcher from Spring,
etc. would have had similar issues as well?
Cheers
--
Manik Surtani
Lead, JBoss Cache
manik(a)jboss.org
9:35 a.m.
New subject: new utility classes for removing Thread.sleeps
Manik Surtani wrote:
Waitaminit, this won't work. JGroups' RpcDispatcher, which
we extend,
doesn't expose a no-arg constructor. Damn. Bela, Vladimir: any
chance of introducing a no-arg constructor for the RpcDispatcher?
Surely others who have tried to use the RpcDispatcher from Spring,
etc. would have had similar issues as well?
Sure. Which versions (2.6 and 2.7 I assume ?) ?
I also assume you'll need getters and setters to inject things like
channels, or listeners.
Can you create a JIRA issue and list the getters/setters you'd like to see ?
--
Bela Ban
Lead JGroups / JBoss Clustering team
JBoss - a division of Red Hat
9:52 a.m.
New subject: new utility classes for removing Thread.sleeps
On 29 May 2008, at 15:35, Bela Ban wrote:
Manik Surtani wrote:
> Waitaminit, this won't work. JGroups' RpcDispatcher, which we
> extend, doesn't expose a no-arg constructor. Damn. Bela,
> Vladimir: any chance of introducing a no-arg constructor for the
> RpcDispatcher? Surely others who have tried to use the
> RpcDispatcher from Spring, etc. would have had similar issues as
> well?
Sure. Which versions (2.6 and 2.7 I assume ?) ?
Not urgent, this is only to help with cleaner unit tests.
I also assume you'll need getters and setters to inject things
like
channels, or listeners.
Yes, Basically everything that can be passed into the ctor should
also have setters.
Can you create a JIRA issue and list the getters/setters you'd
like
to see ?
Will do!
--
Manik Surtani
Lead, JBoss Cache
manik(a)jboss.org
6025
days inactive
6025
days old
jbosscache-dev@lists.jboss.org
4 comments
3 participants
participants (3)
-
Bela Ban -
Manik Surtani -
Mircea Markus