REMReM generate to be able to control the ER lookups

[durga-vasaadi]

Applicable Issues

Description of the Change

REMReM to be able to control the ER lookups

While querying to eventRepository from REMReM added two new parameters that is lookupInExternalERs and lookupLimit

if the value of lookupInExternalERs is set to True then REMReM will query external ERs if the value is set to be False is to decrease the load on external ERs

lookupLimit for displaying the number events

Alternate Designs

Benefits

Possible Drawbacks

Sign-off

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Signed-off-by:

[e-backmark-ericsson]

Overall comment regarding the default values of these two new parameters, as well as for the existing lookup related parameters failIfMultipleFound and failIfNoneFound. As long as all four of them are configurable by config file parameters (application.properties?) I don't care much what their default values are. But my recommendation would be the following: failIfMultipleFound=true failIfNoneFound=true connectToExternalERs=true limit=1 For backwards compatibility reasons it could be better to set the defaults to other values, but you probably know that better than me.

[e-backmark-ericsson]

Sigh, the names of these new parameters might not be as good as I first thought. If it is still easy to change I'd propose to rename:

• connectToExternalERs to lookupInExternalERs
• limit to lookupLimit

Could I get some votes on this suggestion? Is it worth it to rename them?

[tobiasake]

Overall comment regarding the default values of these two new parameters, as well as for the existing lookup related parameters failIfMultipleFound and failIfNoneFound. As long as all four of them are configurable by config file parameters (application.properties?) I don't care much what their default values are. But my recommendation would be the following: failIfMultipleFound=true failIfNoneFound=true connectToExternalERs=true limit=1 For backwards compatibility reasons it could be better to set the defaults to other values, but you probably know that better than me.

If we think there is a use case for having default values configurable in application.properties, then we should make it configureable from application.properties. But, we should not force user to have those parameters in the applications.properties. If these parameters is not available in application.properties, then Java code should default these parameters to the default value that is most common for a general Eiffel usage. That is my opinion how it should be handled. Is that a acceptable solution/implementation for handling default values?

[tobiasake]

Regarding parameter names,, pick names that describes what its used for.

[tobiasake]

Ok,, I was asked to add some comments of these four parameters names: failIfMultipleFound=true failIfNoneFound=true connectToExternalERs=true limit=1

My suggestion the names: failIfMultipleEventsFound failIfNoEventsFound useExternalEventRepositories maxEventsLookupLimit or maxEventsLookupLimitation

[sudharshan-bandaru]

Overall comment regarding the default values of these two new parameters, as well as for the existing lookup related parameters failIfMultipleFound and failIfNoneFound. As long as all four of them are configurable by config file parameters (application.properties?) I don't care much what their default values are. But my recommendation would be the following: failIfMultipleFound=true failIfNoneFound=true connectToExternalERs=true limit=1 For backwards compatibility reasons it could be better to set the defaults to other values, but you probably know that better than me.

If we think there is a use case for having default values configurable in application.properties, then we should make it configureable from application.properties. But, we should not force user to have those parameters in the applications.properties. If these parameters is not available in application.properties, then Java code should default these parameters to the default value that is most common for a general Eiffel usage. That is my opinion how it should be handled. Is that a acceptable solution/implementation for handling default values?

Yes I agree if the user is not configured in application.properties file java code would to take these hard coded default values.

We are going handle accepting default values from application.properties in another request we required some more investigation for this.

[sudharshan-bandaru]

Ok,, I was asked to add some comments of these four parameters names: failIfMultipleFound=true failIfNoneFound=true connectToExternalERs=true limit=1

My suggestion the names: failIfMultipleEventsFound failIfNoEventsFound useExternalEventRepositories maxEventsLookupLimit or maxEventsLookupLimitation

Emil backMark has given the suggestion to change the name of the parameters from connectToExternalERs to lookupInExternalERs and limit to lookupLimit

[sudharshan-bandaru]

Sigh, the names of these new parameters might not be as good as I first thought. If it is still easy to change I'd propose to rename:

• connectToExternalERs to lookupInExternalERs
• limit to lookupLimit

Could I get some votes on this suggestion? Is it worth it to rename them?

Agreed

[e-backmark-ericsson]

I'm fine with this PR now. I will write a follow-up issue for setting the default values using config files. @tobiasake , you've requested changes. Is that still the case? @magnusbaeck , are you fine with this PR as it is now?

[magnusbaeck]

We might as well fix the description in RemremGenerateServiceConstants.LIMIT so it matches what's in the .md file (but preferably also clarifying what happens when the limit is reached). LGTM otherwise.

[sudharshan-bandaru]

The PR description doesn't reference an issue and contains basically no information about why this change is being done. Please also update wiki/markdown/usage/service.md.

Changed the description and updated the service.md also

[sudharshan-bandaru]

We might as well fix the description in RemremGenerateServiceConstants.LIMIT so it matches what's in the .md file (but preferably also clarifying what happens when the limit is reached). LGTM otherwise.

It doesn't have any impact limit will works as pageSize in eventRepository

[magnusbaeck]

It doesn't have any impact

Of course there's an impact if the limit is reached! A user that cares at all about which links are included in the generated message needs to know how REMReM Generate behaves when the limit is reached.

limit will works as pageSize in eventRepository

Nobody outside Ericsson knows anything about pageSize in eventRepository.

[sudharshan-bandaru]

It doesn't have any impact

Of course there's an impact if the limit is reached! A user that cares at all about which links are included in the generated message needs to know how REMReM Generate behaves when the limit is reached.

limit will works as pageSize in eventRepository

Nobody outside Ericsson knows anything about pageSize in eventRepository.

sorry, lookupLimit is similar to limit in mongoDB.