Skip to content
This repository has been archived by the owner on Oct 20, 2022. It is now read-only.

Update EventSinkListener.java #65

Closed
wants to merge 9 commits into from

Conversation

AhmadElseify
Copy link

@AhmadElseify AhmadElseify commented Jan 25, 2018

No description provided.

a part is added to the update method in the EventSinkListener where the provider URI containing the service and  servicePath is retrieved through the epServiceProvider interface and then the service and the servicePath are added to the headers of the EventOut request
@AhmadElseify
Copy link
Author

AhmadElseify commented Jan 26, 2018

@marcc-orange
When i created the pull request , the CI build fails due to nullPointer exception because of one of the test files and i made some attempts in that file to overcome this exception but that didn't work , although the CEP is tested successfully on my side . So , how could i make the pull successfull

@marcc-orange
Copy link
Contributor

marcc-orange commented Jan 26, 2018

I'm sorry, but this spliting of the service provider URL is not the proper way to add serviceName/Path to the updateContext request...

You submission is removing the serviceName/Path set by the Broker at configuration level (this is why tests are broken).

You mentioned in your issue #64 that multi-tenancy was not working for you. I expected a fix concerning the multi-tenancy code, not a modification of the way the serviceName/Path are retrieved from the current configuration.

Currently, the updateContext will be sent to each Broker set in the configuration.
If the Broker configuration contains a serviceName / servicePath, they will be sent as HTTP headers in the request.
I do not understand why you would like to change this behavior in any way.

@AhmadElseify
Copy link
Author

AhmadElseify commented Jan 26, 2018

@marcc-orange
We consider it a multitenancy issue since it's complementary to the multitenancy , because we uploaded multiple configuration files , each with a service and servicepath as mentioned in issue #64 and when we send an incoming request to the CEP with any service and servicePath , the CEP does not check the brokers field of the right config file (the file with the same service and servicePath) and outputs a request with the service and servicePath in the brokers section of the last uploaded config file only .

So we wanted a way to make the CEP outputs a request with the same service and servicePath of the incoming request and not the service and servicepath set in the brokers section in the last uploaded config file . we know it's not the best practice but it will forward the request with service and servicePath of the right configuration and make us avoid being stuck with the service and servicePath in only the last uploaded config file .
Currently , the CEP does not output a request with the service and servicePath in the brokers section of the targeted config file in the multitenancy mode .

@marcc-orange
Copy link
Contributor

the CEP does not check the brokers field of the right config file (the file with the same service and servicePath) and outputs a request with the service and servicePath in the brokers section of the last uploaded config file only .

You certainly found a bug in the multi-tenancy code, but sorry I have no time to try to reproduce it and fix it. Your proposed solution is not the fix to a multi tenancy error.

You can see from the absence of tests in /cepheus-cep/src/test/java/com/orange/cepheus/cep/tenant that the multi-tenancy and complete lack of documentation that it is untested feature.

I would be very glad if you could provide a real fix to the issue and not a workaround that impacts the "normal" usage of the CEP and the current test suite.
I will not merge your proposal as is. Sorry.

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