Thank you Justin!

Attached file contains example configuration I have been using for my test.

Tomo

Justin Bertram wrote:

 

1. A JGROUPS broadcast-type accepts params with these keys:
   • jgroups-configuration-file (defaults to null - user must supply this)
   • jgroups-channel-name (defaults to "hornetq-jgroups-channel")

...

1. A JGROUPS discovery-group accepts params with these keys:
   • jgroups-configuration-file (defaults to null - user must supply this)
   • jgroups-channel-name (defaults to "hornetq-jgroups-channel")

Must the configuration file and channel name be the same for broadcast and discovery or can they be different?

In both cases, I wonder if it'd not be preferable to have a single component with the config file and channel name defined and referenced them in both broadcast type and discovery group.

If the jgroups configuration is different for broadcast & discovery, the 2 configs could be defined next to each other and referenced one by the broadcast type and the other by the jgroups discovery.

This way jgroup elements would be very similar to acceptors and connectors. We define them once in the HornetQ config and reference them everywhere they are required.

wdyt?

makes sense to me Jeff

I haven't looked at the code yet. But do we really need to subclass ServerLocator? My understanding is that we only need to solve how the connector information is broadcasted and accepted over different broadcast layers, like jgroups or UDP. Up that level it should remain the same mechanism. The ServerLocator doesn't need to know from where it gets the connector information or how the Broadcast group sends those information.

Hi Jeff,

I think it's possible, but wondering is it good to have JGroups specific element in the configuration, because the jgroups-discovery is not core stuff but just plugin. It only affects to the config file and FileConfigurationParser, so is not so big deal though.

Hi Howard,

Yeah, I thought so and tried to find out how can I make deal with it without splitting the ServerLocator, but it was tightly-coupled to the UDP & Static discovery and no good idea has come to my mind.

Hi Tomohisa and Justin,

I'm taking to bring Tomohisa's implementation into our repo. First thing I do is make a working branch. Here is what I do:

1. fetch Tomohisa's branch (HORNETQ-316) to my local.

2. As Tomohisa's commit sits on the top but the whole branch falls way back to the master branch. I removed Tomohisa's commit and did a 'git pull --rebase upstream master' to bring the branch up-to-date.

3. Then I did a 'git merge' to merge Tomohisa's commit back to my branch. There are some conflicts I'm trying to solve by hand.

Next I'll review the code and make it ready for pull. Some of the concerns that pops up in my head now are:

1. In such a way will it lose any history?

2. How to maintain backward compatibility? or do we need to?

Apart from that I don't have much thought yet. I'll keep you guys notified of the progress and issues.

1.. you mean from Tomohisa's branch. You can rebase as much as you want, and as much as it makes sense. You just need to make @author tags for things you and him have done in common.

2. if not using JGroups, it should be back compatible. for that reason I was a little concerned on the sub-classes introduced.

Hi Howard,

I have rebased it agaist current upstream so that you can git remote add & cherry-pick easily.

https://github.com/igarashitm/hornetq/tree/HORNETQ-316

https://github.com/igarashitm/hornetq/commit/620f3e70992350088ee6703be5ec0f957ef4deba

Thanks,

Tomo

Thanks Tomo.

Howard

Hi Tomohisa,

I'm using your branch (HORNETQ-316) for code review. I think we use this branch as the base for further code changes and tests. Once we are done then merge the code to master directly from it.

I have some thought regarding the current implementation and would like to discuss:

1. The static way has been unified into DiscoveryGroup concepts. Which I think is a good idea. However the configuration seems not pluggable: The class names for the implementations, like JGroupsDiscoveryGroupImpl, are hard-coded. I'd suggest to make it configurable in the configuration file. Same is true with BroadcastGroup classes.

2. About the ServerLocator. Currently it has an AbstractServerLocator from which a couple of sub-classes are provided, such as StaticServerLocatorImpl and JGroupsServerLocatorImpl. I believe there is a way to simplify it a bit. I'm trying to make it still only one type of ServerLocator, within which it can do its work with different DiscoveryGroups. I'm trying to make some code refactoring at my local branch and see how it goes.

What do you think?

Howard

1.. you mean from Tomohisa's branch. You can rebase as much as you want, and as much as it makes sense. You just need to make @author tags for things you and him have done in common.

Before merging, I'll use Tomohisa's branch as working branch (collaborate with Tomohisa).

2. if not using JGroups, it should be back compatible. for that reason I was a little concerned on the sub-classes introduced.

I'm trying to work with Tomohisa to make sure we are back compatible.

-Howard

Hi Howard,

1. The static way has been unified into DiscoveryGroup concepts. Which I think is a good idea. However the configuration seems not pluggable: The class names for the implementations, like JGroupsDiscoveryGroupImpl, are hard-coded. I'd suggest to make it configurable in the configuration file. Same is true with BroadcastGroup classes.

I agree this is an issue. But putting class name directly into configuration forces user to know fully qualified class name, so it has another pain.

One thing come to my mind is to modulize the hornetq configuration schema. just have a abstract discovery/broadcast element in hornetq-configuration.xsd, and create a hornetq-configuration-static.xsd, hornetq-configuration-udp.xsd, hornetq-configuration-jgroups.xsd and extend that abstract discovery/broadcast schema. (SwitchYard does that kind of schema inheritance and I thought we also can do it for HornetQ) How about that?

2. About the ServerLocator. Currently it has an AbstractServerLocator from which a couple of sub-classes are provided, such as StaticServerLocatorImpl and JGroupsServerLocatorImpl. I believe there is a way to simplify it a bit. I'm trying to make it still only one type of ServerLocator, within which it can do its work with different DiscoveryGroups. I'm trying to make some code refactoring at my local branch and see how it goes.

Great, so you found a cool way that doesn't need to divide ServerLocatorImpl right? Could you share it after you finish please?

And feel free to ask me to adjust the testcases. I can do that

Thanks,

Tomo

Tomohisa igarashi wrote:

 

Hi Howard,

 

1. The static way has been unified into DiscoveryGroup concepts. Which I think is a good idea. However the configuration seems not pluggable: The class names for the implementations, like JGroupsDiscoveryGroupImpl, are hard-coded. I'd suggest to make it configurable in the configuration file. Same is true with BroadcastGroup classes.

I agree this is an issue. But putting class name directly into configuration forces user to know fully qualified class name, so it has another pain.

One thing come to my mind is to modulize the hornetq configuration schema. just have a abstract discovery/broadcast element in hornetq-configuration.xsd, and create a hornetq-configuration-static.xsd, hornetq-configuration-udp.xsd, hornetq-configuration-jgroups.xsd and extend that abstract discovery/broadcast schema. (SwitchYard does that kind of schema inheritance and I thought we also can do it for HornetQ) How about that?

 

 

-- Using abstract xsd seems a bit complicated to me. Can you give more details on that?

My idea is that we need to make it backward compatible. i.e. old configuration should work with the new implementation without changes. I'm trying to finish a prototype based on your code. I'll let you know when it's done. Regarding class names in the configuration file, I think it could be fine as we already done similar thing with connectors/acceptors configuration.

 

2. About the ServerLocator. Currently it has an AbstractServerLocator from which a couple of sub-classes are provided, such as StaticServerLocatorImpl and JGroupsServerLocatorImpl. I believe there is a way to simplify it a bit. I'm trying to make it still only one type of ServerLocator, within which it can do its work with different DiscoveryGroups. I'm trying to make some code refactoring at my local branch and see how it goes.

Great, so you found a cool way that doesn't need to divide ServerLocatorImpl right? Could you share it after you finish please?

 

-- I'll give the details once I get some confidence with it.

 

And feel free to ask me to adjust the testcases. I can do that

 

-- Thanks for that. I'm sure I'll need your help on testing. Especially jgroups stuff.

 

 

Thanks,

Tomo