Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API]: Endpoint for retrieving all policies #1698

Closed
wants to merge 9 commits into from

Conversation

Altair-Bueno
Copy link
Contributor

See #1683

*/
@Immutable
@JsonParsableCommand(typePrefix = ThingSearchCommand.TYPE_PREFIX, name = QueryPolicies.NAME)
public final class QueryPolicies extends AbstractCommand<QueryPolicies> implements ThingSearchQueryCommand<QueryPolicies> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be wise to use an abstract class to avoid code duplication, but:

  1. I'm not sure what level of abstraction you are aiming for
  2. Not even sure if it makes sense, just doing some rough changes until I learn the codebase

Copy link
Member

Choose a reason for hiding this comment

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

I would say to wait for abstracting it before it really provides a benefit, e.g. in reacting to the command in a similar way when applying the things/policies query.

@Altair-Bueno
Copy link
Contributor Author

Hey @thjaeckle . I've been struggling to comprehend the control flow of the GET /search/things endpoint, and I'll like to summarize the steps to make sure I'm not leaving anything important on the table:

  1. An incoming request to said endpoint is transformed into a Ditto command (QueryThings).
  2. The SearchActor handles the command by validating it and retrieving all matching IDs from the persistence layer.
  3. The retrieved IDs are then passed to QueryThingsPerRequestActor.
  4. QueryThingsPerRequestActor instructs ThingsAggregatorProxyActor to retrieve all the corresponding things.
  5. Finally, the ThingsAggregatorProxyActor gathers the requested things and responds back.

However, I'm curious as I don't understand the reasoning behind this approach. It appears quite complex and involves multiple messages between distributed services. Have you considered the possibility of using a more straightforward approach? On paper, a database query seems to be more performant and resource-efficient, as it would only require one microservice and the optimised database (in the end, Mongo is designed for high performance reads).

PS: This comment really help me out to finally gasp where things came from

* The ThingsSearchPersistence returns only Thing IDs. Thus, to provide complete Thing information to the requester,

@thjaeckle
Copy link
Member

thjaeckle commented Jul 25, 2023

@Altair-Bueno this approach is basically done in order to not transfer 200 search results in a single message across the cluster (with a max. thing size of 100 KiB this would be a 20 MiB message).
We configured a max. message size in the cluster of 256 KiB in order to e.g. have an optimized memory consumption and not need to "reserve" a lot of non-needed memory for transfering huge messages.

The solution to still be able to retrieve (max page size) 200 results in a single page at the search API is to

  1. use the search index to find the "thingIds" which match the query
  2. respond with the list of "thingIds"
  3. then 1-by-1 ask for the data of the things from the "things" service
    • this also will "filter" the responses based on the policy to not have data included which the user is not allowed to see
  4. aggregate them back at the gateway into a single "QueryThingsResponse"

So yes, it is complex - but it is required to not exceed messaging limits.
Policies can also be 100 KiB big (default configuration) - so the same approach has to be applied here as well I fear.

@Altair-Bueno
Copy link
Contributor Author

Altair-Bueno commented Jul 26, 2023

Thanks for your response Thomas. I do understand now the motivation behind performing multiple asks to the things service instead a single big one. However, that doesn't explain why asking each actor was chosen over a database query from the gateway component itself.

The only reason I can think of is to avoid stale data to be served (e.g. a thing with changes that hasn't been persisted yet), but IMHO that seems acceptable given the drawbacks of the current approach: increased pressure on Akka, latency and software complexity. If one would like a more precise representation of the actual twin, a GET /things/{thingid} query could performed to retrieve the latest changes.

Please keep in mind that I have little experience with Akka/actors and this might be common practice in actor systems. It just doesn't click with me to rely so heavily on actors when other approaches exists, specially when we are already relying on the database for the ID query.

@thjaeckle
Copy link
Member

However, that doesn't explain why asking each actor was chosen over a database query from the gateway component itself.

As I wrote:

  • this also will "filter" the responses based on the policy to not have data included which the user is not allowed to see

The search index does not necessarily have all of the information which the thing contains - its purpose is "only" to return with a list of matching IDs to a query.
With this approach, it is also possible to e.g. limit the fields which the search index should index as discussed in #1521

As things and policies apply the event sourcing pattern (e.g. in order to provide history and audit log capabilities), we cannot "simply" query their database from the gateway (as the actual state would have to be constructed based on persisted events) - which you should not do anyways in service oriented architectures - to use the database of another service without the other service being aware.

This is not going to change or be simplified as I don't see how this can work otherwise.

@thjaeckle
Copy link
Member

@Altair-Bueno this is no longer in work, or is it?

@Altair-Bueno
Copy link
Contributor Author

Hi Thomas. Indeed, we are no longer working on this. I've recently been reassigned, and unfortunately, this feature development slipped through the cracks. I apologize for any inconvenience this may have caused.

@thjaeckle
Copy link
Member

@Altair-Bueno ok, no problem - thanks for letting us know 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants