Skip to content
This repository was archived by the owner on May 9, 2019. It is now read-only.

Replace ItemService.getUsers #120

Closed
TimMoore opened this issue Jun 30, 2017 · 2 comments
Closed

Replace ItemService.getUsers #120

TimMoore opened this issue Jun 30, 2017 · 2 comments
Assignees

Comments

@TimMoore
Copy link
Contributor

Right now, the getUsers service implementation is not ideal:

        // Note this should never make production....
        return req -> currentIdsQuery.currentPersistenceIds()
                .filter(id -> id.startsWith("PUserEntity"))
                .mapAsync(4, id -> entityRef(id.substring(11)).ask(PUserCommand.GetPUser.INSTANCE))
                .filter(Optional::isPresent)
                .map(user -> Mappers.toApi(user.get()))
                .runWith(Sink.seq(), mat)
                .thenApply(TreePVector::from);

As you can tell from the comment, this code is a quick hack... not something that should be considered a good example of writing a Lagom service.

It has a few problems:

  • It is slow. This requires loading each user entity into memory to query it for its details. In a multi-node cluster, this will require multiple calls across the network.
  • It is fragile. It assumes too much about the format of the persistence ID string. See Fresh master clone can't be used #115 for an example of the problems this causes
  • It does not scale well. In a realistic system with a large number of users, this will return a very long list with no pagination.
  • It is written in a very functional style that may be unfamiliar to many Java developers.

This service call is used by the web-gateway application to load a list of all users in the Nav. This is then used in turn for different purposes:

  1. To create a list of users in the navigation bar, which allows you to log in as different users
  2. To look up user details given a user ID, as an optimization of calling userService.getUser for each ID, since all of the user information was already loaded and cached locally for the navigation bar

These can be replaced by different means.

Navigation bar

In the navigation bar, it might be better to show a bounded list of recently-created users. For example, we could put the ten most recently-created users in the list, rather than all of the users in the system. For most demo purposes, it will be exactly the same, since most people won't create that many users. However, if we decide to extend the demo to create users automatically (see #110 for example) then we might want to include a script that creates many more than ten users.

This is a perfect case for a read-side table in Lagom. This would look similar to the existing read-side processor in the item service and the one currently under review in the transaction service (#106). It would maintain a table of users and their creation date that could be queried by the service implementation in a new getRecentUsers service call. We would need to extend the PUserCreated event to carry a createdAt timestamp.

User lookups

I think that using the navigation to look up user details, rather than making calls to userService.getUser(id), could be considered a premature optimization. I suggest we undo that and stick with making the calls for now.

If we want to demonstrate optimizations in the future, we have a few options:

  1. We could keep calling getUser(id) but maintain a local cache, so it doesn't have to be called as often
  2. We could replace the getUsers() call that returns all users with a call that takes a list of user IDs to return
  3. We could publish user events to Kafka, and have the web-gateway listen to these events and maintain its own internal copy of user display names
@lakhina lakhina self-assigned this Jul 13, 2017
@lakhina
Copy link
Contributor

lakhina commented Jul 13, 2017

I am working on adding readside in user. I will work on it as soon as that PR is complete and merged.

@TimMoore
Copy link
Contributor Author

TimMoore commented Aug 8, 2017

The first part of this was fixed in #131.

The second part has been explained in more detail in #145, so this issue can be closed.

@TimMoore TimMoore closed this as completed Aug 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

2 participants