Skip to content

Move service discovery to objectsmanager and some clean up (QC-193, QC-189)#184

Merged
Barthelemy merged 6 commits intoAliceO2Group:masterfrom
Barthelemy:fix-service-discovery
Jun 17, 2019
Merged

Move service discovery to objectsmanager and some clean up (QC-193, QC-189)#184
Barthelemy merged 6 commits intoAliceO2Group:masterfrom
Barthelemy:fix-service-discovery

Conversation

@Barthelemy
Copy link
Collaborator

No description provided.

@Barthelemy Barthelemy requested review from awegrzyn and knopers8 June 13, 2019 13:51
@knopers8
Copy link
Collaborator

The application is not blocked anymore, but I can see "[30270]: bind: Address already in use" in the logs.

@Barthelemy
Copy link
Collaborator Author

@knopers8 Are you sure it is linked to the Service Discovery ? Do you have something running on port 7777 ? (I think that it is the default)

@knopers8
Copy link
Collaborator

I have checked, nothing is running on that port.

@knopers8
Copy link
Collaborator

I am pretty sure it is connected somehow to Service Discovery. When it was getting stuck before, it was always stuck in that place.

@Barthelemy
Copy link
Collaborator Author

Should I merge nevertheless ? the test failure is unrelated.

@awegrzyn
Copy link
Contributor

I can take a look at it today.

@knopers8
Copy link
Collaborator

It improves the situation, so I am fine with merging this.

mMonitorObjects.SetOwner(true);

// register with the discovery service
mServiceDiscovery = std::make_unique<ServiceDiscovery>("http://consul-test.cern.ch:8500", mTaskName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to move the URL to config file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, let me do that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@Barthelemy
Copy link
Collaborator Author

Barthelemy commented Jun 14, 2019

We duplicate monitoring and consul url in all json files. We should see how to do it better. For now it will do.
@awegrzyn The test testObjectsManager::invalid_url_test shows that the ServiceDiscovery hangs if I provide a wrong url. It hangs in the destructor of ServiceDiscovery (during join) when ObjectsManager exits.
Could we have a timeout ? a check on the url ? a fix ?

@Barthelemy Barthelemy changed the title Move service discovery to objectsmanager and some clean up (QC-193, QC-189) [WIP] Move service discovery to objectsmanager and some clean up (QC-193, QC-189) Jun 14, 2019
@awegrzyn
Copy link
Contributor

Sure but when I just run o2-qc-run-basic it does not happen, what argument shall I pass in order to set URL?

@Barthelemy
Copy link
Collaborator Author

@awegrzyn I think that it would be better to work with the test because it excludes the DPL.
I'll pass by your office.

@Barthelemy
Copy link
Collaborator Author

@awegrzyn To run the test do

cd alice/sw/BUILD/QualityControl-latest/QualityControl
./bin/testObjectsManager

It hangs (even after your commit).

@awegrzyn awegrzyn force-pushed the fix-service-discovery branch from dc06ed4 to 1a4111a Compare June 17, 2019 13:27
@Barthelemy
Copy link
Collaborator Author

@awegrzyn works well! Thank you
I have fixed the formatting and wait for the tests. If everything is green, I will merge it.

@awegrzyn
Copy link
Contributor

Ok, but we need to verify whether it works well in the real scenario.

@Barthelemy
Copy link
Collaborator Author

Yes, but the plan is to merge , then release, then let you play with it.

@Barthelemy Barthelemy changed the title [WIP] Move service discovery to objectsmanager and some clean up (QC-193, QC-189) Move service discovery to objectsmanager and some clean up (QC-193, QC-189) Jun 17, 2019
@Barthelemy Barthelemy merged commit 3333d46 into AliceO2Group:master Jun 17, 2019
@Barthelemy Barthelemy deleted the fix-service-discovery branch July 2, 2019 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants