Add readside processor to user #131

lakhina · 2017-07-13T09:24:18Z

Phase one of #120 is complete. Now getUsers() provide you 10 users in nav list in with alphabatically least uuid

TimMoore

I would recommend writing a test for this, like the ones for ItemRepository and TransactionRepository, so that it will be easier to catch bugs more quickly.

TimMoore · 2017-07-14T00:31:08Z

user-impl/src/main/java/com/example/auction/user/impl/PUser.java

@@ -2,6 +2,7 @@

 import com.lightbend.lagom.serialization.Jsonable;
 import lombok.Value;
+import lombok.experimental.Wither;


Is it used?

TimMoore · 2017-07-14T00:31:45Z

user-api/src/main/java/com/example/auction/user/api/UserService.java

+        ).withPathParamSerializer(
+                UUID.class, PathParamSerializers.required("UUID", UUID::fromString, UUID::toString)
+        ).withHeaderFilter(SecurityHeaderFilter.INSTANCE);
+      //  ).withPathParamSerializer(UUID.class, PathParamSerializers.required("UUID", UUID::fromString, UUID::toString));


The commented line can be deleted, since it is duplicated above.

TimMoore · 2017-07-14T00:40:44Z

user-impl/src/main/java/com/example/auction/user/impl/UserRepository.java

+
+                        user.getId(),
+                        user.getEmail(),
+                        user.getName()


This doesn't look quite right. The statement you are binding to is this:

"INSERT INTO User(UserId, Name, email, PasswordHash) VALUES (?, ?, ?, ?)"

You need to bind the same number of parameters, in the same order they appear in the statement.

However, I would also say that you do not need to include the PasswordHash column in this table at all, since it is not used on the read side.

TimMoore · 2017-07-14T00:42:54Z

user-impl/src/main/java/com/example/auction/user/impl/UserRepository.java

+
+            private CompletionStage<List<BoundStatement>> insertUser(PUser user) {
+                return completedStatements(
+                        (List<BoundStatement>) insertUserCreator(user)


This cast will fail: a BoundStatement cannot be cast to a List<BoundStatement>. You will need to wrap it in a list instead, such as Arrays.asList(insertUserCreator(user)).

TimMoore · 2017-07-14T00:46:30Z

user-impl/src/main/java/com/example/auction/user/impl/UserRepository.java

+                                        "UserId UUID PRIMARY KEY, " +
+                                        "Name text, "+
+                                        "email text, " +
+                                        "PasswordHash text "+


As mentioned in another comment, there's no need for the PasswordHash column, since it is never read.

TimMoore · 2017-07-14T00:49:55Z

user-api/src/main/java/com/example/auction/user/api/UserService.java

+                pathCall("/api/user?pageNo&pageSize", this::getUsers)
+        ).withPathParamSerializer(
+                UUID.class, PathParamSerializers.required("UUID", UUID::fromString, UUID::toString)
+        ).withHeaderFilter(SecurityHeaderFilter.INSTANCE);


What's the reason for adding the SecurityHeaderFilter? I don't see it used anywhere.

TimMoore · 2017-07-14T00:50:44Z

user-impl/src/main/java/com/example/auction/user/impl/UserRepository.java

+            private CompletionStage<Done> prepareInsertUserStatement() {
+
+                return session.
+                        prepare("INSERT INTO User(" +


The name of the table here ("User") does not match the table created above ("UserInfo").

TimMoore · 2017-07-14T00:51:04Z

user-impl/src/main/java/com/example/auction/user/impl/UserRepository.java

+                );
+            }
+            private CompletionStage<Optional<Row>> selectUser(UUID UserId) {
+                return session.selectOne("SELECT * FROM UserCreator WHERE UserId = ?", UserId);


This table name also does not match either of the other two named used.

TimMoore · 2017-07-18T05:47:46Z

user-impl/src/main/java/com/example/auction/user/impl/UserRepository.java

+                            "CREATE TABLE IF NOT EXISTS UserInfo (" +
+                                    "UserId UUID PRIMARY KEY, " +
+                                    "Name text, " +
+                                    "email text, " +


I'm not sure what happened to the comment from @yg-apaza pointing out the same, but there is a syntax error here: trailing comma on the last column.

@TimMoore @yg-apaza Thanks. I will fix it.

This is not a syntax error, on the contrary.

If I remove the trailing comma I get a syntax error from the cassandra driver.
ItemRepository also have a comma separating the last column and the definition of a compound primary key.

There is two slightly different syntax.

CREATE TABLE demo ( foo text PRIMARY KEY, bar text )

No comma between foo column definition and PRIMARY KEY as we are marking foo as the primary key.

CREATE TABLE demo ( foo text, bar text, PRIMARY KEY(foo, bar) )

In the above example we do need a comma because we are defining a compound PK. In that case, "PRIMARY KEY" is not part of the column definition and therefore a comma is needed

@TimMoore, I think that @yg-apaza's comment disappeared because she may have realised it and removed it.

@rcavalcanti I have changed this query now.

hmm, seems that I had already checked out the one with the compound key that made the comments here obsolete. Sorry for the noisy on the line.

TimMoore · 2017-07-20T04:44:44Z

user-api/src/main/java/com/example/auction/user/api/User.java

    private final String name;
    private final String email;

    @JsonCreator
-    public User(@JsonProperty("id") UUID id, @JsonProperty("name") String name, @JsonProperty("email") String email) {
+    public User(@JsonProperty("id") UUID id, @JsonProperty("creatorId") UUID creatorId , @JsonProperty("name") String name, @JsonProperty("email") String email) {


I think we want it to be a timestamp (maybe called createdAt) rather than a UUID.

TimMoore · 2017-07-21T05:52:23Z

user-api/src/main/java/com/example/auction/user/api/User.java

@@ -4,19 +4,23 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import lombok.Value;

+import java.sql.Timestamp;
+import java.time.Instant;


I see that you have imported both Timestamp and Instant here... it would be better to use Instant and not Timestamp, because java.sql.Timestamp is specific to JDBC, which we are not using here.

TimMoore · 2017-07-21T05:53:20Z

user-api/src/main/java/com/example/auction/user/api/User.java

    private final String name;
    private final String email;

    @JsonCreator
-    public User(@JsonProperty("id") UUID id, @JsonProperty("name") String name, @JsonProperty("email") String email) {
+    public User(@JsonProperty("id") UUID id, @JsonProperty("createdAt") Timestamp createdAt , @JsonProperty("name") String name, @JsonProperty("email") String email) {


This constructor should be removed, similar to all of the other @Value classes.

TimMoore · 2017-07-21T05:53:59Z

user-impl/src/main/java/com/example/auction/user/impl/PUserCommand.java

 import java.util.Optional;
+import java.util.UUID;


Please be sure to clean unused imports.

TimMoore · 2017-07-21T05:55:19Z

user-impl/src/main/java/com/example/auction/user/impl/PUserCommand.java


 public interface PUserCommand extends Jsonable {
    @Value
    final class CreatePUser implements PUserCommand, PersistentEntity.ReplyType<PUser> {
+        private final Timestamp createdAt;


Rather than putting createdAt in the command, I think it's better to generate it within the persistent entity when a CreatePUser command is received.

TimMoore · 2017-07-21T06:04:21Z

user-impl/src/main/java/com/example/auction/user/impl/UserRepository.java

+            return insertUserStatement.bind(
+
+                    user.getId(),
+                    user.getCreatedAt(),


In order to bind an Instant to a Cassandra query, you can use the optional InstantCodec provided by the Java Cassandra driver.

You can see an example of how to register a codec in ItemRepository (and there is a similar example in TransactionRepository).

TimMoore · 2017-07-21T06:05:18Z

web-gateway/src/main/resources/application.conf

@@ -1,5 +1,5 @@
 play.http.secret.key = "somesecret"
 online-auction.instruction.show = true
-
+lagom.persistence.read-side.global-prepare-timeout = 60s


If this doesn't fix the problem for you, then it should probably be removed. We'll need to find some other solution.

TimMoore · 2017-07-21T06:09:35Z

@lakhina the latest build has a new error:

 Caused by: java.util.concurrent.ExecutionException: java.util.concurrent.ExecutionException: com.datastax.driver.core.exceptions.SyntaxError: line 1:107 mismatched input '(' expecting ')' (..., email text PRIMARY KEY [(]createdAt...)

This indicates a syntax error in your CQL statement. Have a look at the stack trace to find where.

The error "Couldn't register the processor on the testkit" is a generic wrapper exception that could have a variety of different causes. It's important to always read the nested exceptions to find the root cause of the error.

TimMoore

@lakhina can you explain your thinking for "Change reply type of PUserCreated to Optional" please? I don't understand why this change was made.

octonato · 2017-07-27T22:56:36Z

@lakhina I have been trying to trace why the build is failing.

Most of the times it fails with a timeout on the actor system when it tries to create the tables. I could find the cause of that yet. I will keep searching.

However, from time to time it succeeds and when it happens we get other errors on the UserRepositoryTest (see my comments there)

octonato · 2017-07-27T22:58:04Z

user-impl/src/main/java/com/example/auction/user/impl/UserRepository.java

+
+        private CompletionStage<List<BoundStatement>> insertUser(PUser user) {
+            return completedStatements(
+                    Arrays.asList(insertUserCreator(user))


Here we can remove the Arrays.asList because there is an overloaded completedStatements that gets a varargs.

octonato · 2017-07-27T23:01:37Z

user-impl/src/test/java/com/example/auction/user/impl/UserRepositoryTest.java

+        }
+
+        PaginatedSequence<User> createdUsers = Await.result(userRepository.getUsers(1, 10));
+        assertEquals(25, createdUsers.getCount());


Because we insert a User on previous test, the count here will be 26.

We have two options:

make sure table is cleared before each test (costly)

cache the current count before insert the new Users and compare with it.

int size = 25 int initialCount = createdUsers.getCount(); // create users here assertEquals(initialCount + size, createdUsers.getCount());

octonato · 2017-07-27T23:02:05Z

user-impl/src/test/java/com/example/auction/user/impl/UserRepositoryTest.java

+        assertEquals(25, createdUsers.getCount());
+        assertEquals(10, createdUsers.getItems().size());
+        // default ordering is time DESC so page 2 of size 10 over a set of 25 returns item ids 5-14. On that seq, the fifth item is id=10
+        assertEquals("user10", createdUsers.getItems().get(4).getName());


I'm getting admin4 instead. Not user10

lakhina · 2017-07-28T07:48:52Z

@rcavalcanti Thanks for the help. I have made the suggested changes but still the same error is persisting.

octonato · 2017-07-28T08:29:40Z

@lakhina the fix I suggested won't fix the build problem (see previous comment).

What I observed is that when it happens to pass the stage of table creation, it does fail because of errors in the test itself.

Can you push your last changes? Thanks

octonato · 2017-07-28T09:08:10Z

user-impl/src/test/java/com/example/auction/user/impl/UserRepositoryTest.java

    private final PUser userCreated = new PUser(userId, createdAt, name, email, password);
+    int initialCount = createdUsers.getCount();


I think it's better to have it inside the test method instead.

size is the number of users you plan to create inside shouldPaginateUserRetrieval method.
Preferably, we should use this value in the for loop as well.

for (int i = 0; i < size; i++) { ... }

The initialCount as well. This call will happen on class initialisation. At that point, the initial count is 0.

My suggestion was more something like the following:

@Test public void shouldPaginateUserRetrieval() throws InterruptedException, ExecutionException, TimeoutException { PaginatedSequence<User> usersCreatedBefore = Await.result(userRepository.getUsers(1, 10)); int initialCount = usersCreatedBefore.getCount(); int size = 25; for (int i = 0; i < size; i++) { feed(new PUserEvent.PUserCreated(buildFixture(createdAt, i))); } PaginatedSequence<User> createdUsers = Await.result(userRepository.getUsers(1, 10)); assertEquals(size + initialCount, createdUsers.getCount()); }

Looking back to the code I just pasted here, it is even better to call userRepository.countUsers() instead of userRepository.getUsers(1, 10). Certainly for the first call where we are only interested in the total current count.

octonato · 2017-07-28T09:26:05Z

user-impl/src/test/java/com/example/auction/user/impl/UserRepositoryTest.java

        assertEquals(10, createdUsers.getItems().size());
        // default ordering is time DESC so page 2 of size 10 over a set of 25 returns item ids 5-14. On that seq, the fifth item is id=10
-        assertEquals("user10", createdUsers.getItems().get(4).getName());
+        assertEquals("admin4", createdUsers.getItems().get(4).getName());


It seems that I was wrong about this one. I'm getting random values here.
I've seen the following values so far:
admin4
admin10
admin22
admin18

lakhina · 2017-07-28T13:05:16Z

@rcavalcanti I have committed the changes. Please have a look.

octonato · 2017-07-31T16:15:52Z

user-impl/src/main/java/Module.java

 import com.google.inject.AbstractModule;
 import com.lightbend.lagom.javadsl.server.ServiceGuiceSupport;

-public class UserModule extends AbstractModule implements ServiceGuiceSupport {
+public class Module extends AbstractModule implements ServiceGuiceSupport {


I could finally detect the origin of the broken build. I don't have yet full understand how does it happens, but it's related with the order of wiring of Guice modules.

It seems that when using a non-standard location for the Guice module, the bit that will make the akka cluster to join itself is affected by the bootstrap of cassandra. Probably by the loading of the Cassandra Module.

What I have observed is that sometimes (1 over 3 times), the cluster joining (JoinClusterImpl.join) is only called after some Cassandra setup.

2017-07-31 11:17:32,128 [INFO] from akka.cluster.Cluster(akka://ServiceTest_170731111724385) - Cluster Node [akka.tcp://ServiceTest_170731111724385@127.0.0.1:53609] - Starting up... 2017-07-31 11:17:32,214 [INFO] from akka.cluster.Cluster(akka://ServiceTest_170731111724385) - Cluster Node [akka.tcp://ServiceTest_170731111724385@127.0.0.1:53609] - Registered cluster JMX MBean [akka:type=Cluster] 2017-07-31 11:17:32,215 [INFO] from akka.cluster.Cluster(akka://ServiceTest_170731111724385) - Cluster Node [akka.tcp://ServiceTest_170731111724385@127.0.0.1:53609] - Started up successfully 2017-07-31 11:17:32,236 [INFO] from akka.cluster.Cluster(akka://ServiceTest_170731111724385) - Cluster Node [akka.tcp://ServiceTest_170731111724385@127.0.0.1:53609] - No seed-nodes configured, manual cluster join required ... 2017-07-31 11:17:35,577 [DEBUG] from com.datastax.driver.core.ControlConnection - [Control connection] Refreshing schema for servicetest_170731111724385 2017-07-31 11:17:39,781 [INFO] from org.apache.cassandra.auth.CassandraRoleManager - Created default superuser role 'cassandra' // JoinClusterImpl.join is called here 2017-07-31 11:17:54,137 [INFO] from akka.cluster.Cluster(akka://ServiceTest_170731111724385) - Cluster Node [akka.tcp://ServiceTest_170731111724385@127.0.0.1:53609] - Node [akka.tcp://ServiceTest_170731111724385@127.0.0.1:53609] is JOINING, roles [] 2017-07-31 11:17:54,150 [INFO] from akka.cluster.Cluster(akka://ServiceTest_170731111724385) - Cluster Node [akka.tcp://ServiceTest_170731111724385@127.0.0.1:53609] - Leader is moving node [akka.tcp://ServiceTest_170731111724385@127.0.0.1:53609] to [Up]

When this happens, the UserRepositoryTest fails because it depends on async call to ClusterStartupTaskActor in order to get an instance of CassandraOffsetDao which is required by the cassandra read side.

Wow, that is some very good detective work! How strange.

We have noticed some other issues related to this in the past, for example, lagom/lagom#568. That issue pertains to JDBC rather than Cassandra, but I think it's similar in that the underlying issue is a lack of coordination in the initialization order. All of our ClusterStartupTask implementations kick off immediately as soon as they are defined.

@rcavalcanti: could you record your findings in a lagom/lagom issue? We should really fix this upstream.

octonato · 2017-07-31T16:18:32Z

user-impl/src/test/java/com/example/auction/user/impl/UserRepositoryTest.java

+        PaginatedSequence<User> createdUsers = Await.result(userRepository.getUsers(1, 10));
+        assertEquals(size + initialCount, createdUsers.getCount());
+        assertEquals(10, createdUsers.getItems().size());
+        assertEquals("admin11", createdUsers.getItems().get(4).getName());


This one will randomly fail because the ordering is not guaranteed.

UserRepository.selectUsers must include a order by createdAt to make this call deterministic.

octonato · 2017-07-31T16:19:56Z

user-impl/src/test/java/com/example/auction/user/impl/UserRepositoryTest.java

+    public void shouldPaginateUserRetrieval() throws InterruptedException, ExecutionException, TimeoutException {
+
+        PaginatedSequence<User> usersCreatedBefore = Await.result(userRepository.getUsers(1, 10));
+        int initialCount = usersCreatedBefore.getCount();


If we make UserRepository.countUsers() public, we can calculate the initialCount in one shot without the end fetch results in-memory.

int initialCount = Await.result(userRepository.countUsers());

TimMoore · 2017-08-01T02:06:00Z

With the latest changes from @rcavalcanti, it looks like the build failure is now a legitimate test failure (should be reproducible locally) rather than a problem with the framework.

TimMoore

Green build ✅

Thanks for all of your effort on this @lakhina!

lakhina changed the title ~~Add readside processor to user credential~~ [WIP] Add readside processor to user credential Jul 13, 2017

lakhina force-pushed the readside branch 2 times, most recently from 40f14d3 to 5944d45 Compare July 13, 2017 12:09

lakhina changed the title ~~[WIP] Add readside processor to user credential~~ [WIP] Add readside processor to user Jul 13, 2017

lakhina force-pushed the readside branch from 5944d45 to 45c84ec Compare July 13, 2017 14:10

lakhina changed the title ~~[WIP] Add readside processor to user~~ Add readside processor to user Jul 13, 2017

lakhina requested review from jroper, TimMoore and ignasi35 and removed request for jroper and TimMoore July 13, 2017 14:34

lakhina assigned yg-apaza Jul 13, 2017

TimMoore suggested changes Jul 14, 2017

View reviewed changes

lakhina force-pushed the readside branch 5 times, most recently from d8bae49 to cd05b99 Compare July 15, 2017 19:08

TimMoore reviewed Jul 18, 2017

View reviewed changes

TimMoore assigned lakhina and unassigned yg-apaza Jul 18, 2017

lakhina force-pushed the readside branch 2 times, most recently from 1594163 to af92ea0 Compare July 18, 2017 15:47

TimMoore reviewed Jul 20, 2017

View reviewed changes

lakhina force-pushed the readside branch from 8921929 to e53d527 Compare July 20, 2017 17:42

TimMoore reviewed Jul 21, 2017

View reviewed changes

Add creatorAt timestamp in User

7c3beb1

lakhina force-pushed the readside branch from e53d527 to 44be056 Compare July 24, 2017 13:24

Update UserRepository.java

e6a0e0f

lakhina force-pushed the readside branch 2 times, most recently from 427c8d1 to 694a55d Compare July 25, 2017 18:04

TimMoore reviewed Jul 26, 2017

View reviewed changes

lakhina force-pushed the readside branch from 694a55d to 5edbf6e Compare July 26, 2017 13:48

Update selectUsers in UserRepository

895e412

lakhina force-pushed the readside branch from 5edbf6e to 895e412 Compare July 26, 2017 14:56

octonato reviewed Jul 27, 2017

View reviewed changes

octonato reviewed Jul 28, 2017

View reviewed changes

lakhina force-pushed the readside branch from 5c6c297 to e55ebdf Compare July 28, 2017 10:10

Modify UserRepositoryTest

0bc2f55

lakhina force-pushed the readside branch from e55ebdf to 0bc2f55 Compare July 28, 2017 11:32

moves Guice module to default package

476e3be

octonato reviewed Jul 31, 2017

View reviewed changes

lakhina added 2 commits August 1, 2017 11:14

Optimize imports and modify countUsers

efb6f85

Removed ordering based on createdAt

10d86b7

TimMoore approved these changes Aug 2, 2017

View reviewed changes

TimMoore merged commit a5a8b5a into lagom:master Aug 2, 2017

octonato mentioned this pull request Aug 2, 2017

Module loading order may cause wiring failures lagom/lagom#909

Closed

This was referenced Aug 8, 2017

Change single-user lookups by ID in the web-gateway to use the UserService #145

Closed

Replace ItemService.getUsers #120

Closed

		private final PUser userCreated = new PUser(userId, createdAt, name, email, password);
		int initialCount = createdUsers.getCount();

Add readside processor to user #131

Add readside processor to user #131

Conversation

lakhina commented Jul 13, 2017 • edited Loading

TimMoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

octonato Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimMoore commented Jul 21, 2017

TimMoore left a comment

Choose a reason for hiding this comment

octonato commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

lakhina commented Jul 28, 2017

octonato commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakhina commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimMoore commented Aug 1, 2017

TimMoore left a comment

Choose a reason for hiding this comment

lakhina commented Jul 13, 2017 •

edited

Loading

octonato Jul 28, 2017 •

edited

Loading

octonato Jul 28, 2017 •

edited

Loading

octonato Jul 27, 2017 •

edited

Loading