lagom / online-auction-java

Other
129 stars 109 forks source link

Replace ItemService.getUsers #120

Closed TimMoore closed 7 years ago

TimMoore commented 7 years ago

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:

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 commented 7 years ago

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

TimMoore commented 7 years ago

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.