Skip to content
This repository was archived by the owner on Jul 8, 2022. It is now read-only.

improve event subscription#385

Closed
Ingvord wants to merge 21 commits intomasterfrom
improve-event-subscription
Closed

improve event subscription#385
Ingvord wants to merge 21 commits intomasterfrom
improve-event-subscription

Conversation

@Ingvord
Copy link
Copy Markdown
Member

@Ingvord Ingvord commented Jul 28, 2017

Currently each client and server guesses about the channel to use for the event subscription.

In this PR we want to implement handshaking mechanism in which client asks server for the channel.

As this will be back ported into tango-9-lts minimal code refactorings must be applied.

@Ingvord Ingvord self-assigned this Jul 28, 2017
@Ingvord Ingvord force-pushed the improve-event-subscription branch from 844a555 to 153d3bb Compare August 29, 2017 13:25
Ingvord added a commit that referenced this pull request Aug 29, 2017
@tango-controls tango-controls deleted a comment Aug 29, 2017
Ingvord added a commit that referenced this pull request Aug 29, 2017
@Ingvord
Copy link
Copy Markdown
Member Author

Ingvord commented Aug 30, 2017

Looks like cppTango does not set ZMQ topic filter: see my gist

Filtering is performed manually through some non-trivial code, like:

        string local_callback_key(device_name);

	string::size_type pos;
	if ((pos = local_callback_key.find('#')) == string::npos)
	{
		if (inter_event == true)
			local_callback_key = local_callback_key + "." + event_name;
		else
			local_callback_key = local_callback_key + "/" +obj_name_lower + "." + event_name;
	}
	else
	{
		local_callback_key.erase(pos);
		if (inter_event == true)
			local_callback_key = local_callback_key + MODIFIER_DBASE_NO + '.' + event_name;
		else
			local_callback_key = local_callback_key + "/" + obj_name_lower + MODIFIER_DBASE_NO + '.' + event_name;
	}

I guess this was designed to prevent socket being created for every subscription (I wonder if ZMQ reuses sockets underneath)

@Ingvord
Copy link
Copy Markdown
Member Author

Ingvord commented Aug 30, 2017

a few hundred lines below:

//
// Subscribe to the new event
//

            event_sub_sock->setsockopt(ZMQ_SUBSCRIBE,event_name,::strlen(event_name));

so ZMQ topics are actually used...

Ingvord added a commit that referenced this pull request Aug 30, 2017
Ingvord added a commit that referenced this pull request Aug 30, 2017
@Ingvord Ingvord force-pushed the improve-event-subscription branch from 4a83964 to a482580 Compare August 30, 2017 14:18
@tango-controls tango-controls deleted a comment Aug 30, 2017
@tango-controls tango-controls deleted a comment Aug 31, 2017
@tango-controls tango-controls deleted a comment Aug 31, 2017
@tango-controls tango-controls deleted a comment Aug 31, 2017
{
namespace admin
{
namespace commands
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello!
This part looks very Java-ish to me ;-)...
Do we really need so many nested namespaces? I would drop some of them to improve readability of the code.

Copy link
Copy Markdown
Member Author

@Ingvord Ingvord Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, you are right... perhaps org::tango::common would be enough.

P.S. once we switch to C++17 we can replace this with namespace org::tango::common::admin::commands {...}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would even remove the 'org' part. I don't think we need it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will remove org but leave the rest to be consistent with the folder structure. We will see how this evolve and maybe change in the future.

@tango-controls tango-controls deleted a comment Sep 1, 2017
@tango-controls tango-controls deleted a comment Sep 1, 2017
@tango-controls tango-controls deleted a comment Sep 1, 2017
@tango-controls tango-controls deleted a comment Sep 1, 2017
@tango-controls tango-controls deleted a comment Sep 2, 2017
@Ingvord
Copy link
Copy Markdown
Member Author

Ingvord commented Oct 25, 2017

I do not like where it goes. So I close this one and start another #404

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants