Institutions cassandra #1013

jmarin · 2017-06-13T13:30:20Z

Closes #1012
Doesn't remove PostgreSQL implementation yet

codecov-io · 2017-06-13T16:04:40Z

Codecov Report

❗ No coverage uploaded for pull request base (master@29021da). Click here to learn what that means.
The diff coverage is 80.95%.

@@            Coverage Diff            @@
##             master    #1013   +/-   ##
=========================================
  Coverage          ?   93.13%           
=========================================
  Files             ?      291           
  Lines             ?     3830           
  Branches          ?       73           
=========================================
  Hits              ?     3567           
  Misses            ?      263           
  Partials          ?        0

Impacted Files	Coverage Δ
api/src/main/scala/hmda/api/HmdaPlatform.scala	`0% <0%> (ø)`
.../institutions/InstitutionCassandraProjection.scala	`0% <0%> (ø)`
...ry/src/main/scala/hmda/query/CassandraConfig.scala	`100% <100%> (ø)`
.../institutions/InstitutionCassandraRepository.scala	`100% <100%> (ø)`
...la/hmda/query/repository/CassandraRepository.scala	`56.25% <56.25%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29021da...1172811. Read the comment docs.

schbetsy · 2017-06-14T15:26:52Z

Smoke tests for this PR look good, both locally and on Docker-Compose. I still need to take a closer look at the code, but I wanted to say 👍 so far.

I did notice a long keycloak stack trace in the docker-compose logs that started with the following line. I'm not sure it's relevant to this PR, but I wanted to call it out.

keycloak_1     | 14:23:36,222 ERROR [org.keycloak.services] (default task-14) KC-SERVICES0067: 
    failed to parse RestartLoginCookie: java.lang.RuntimeException: 
    java.lang.RuntimeException: java.security.InvalidKeyException: key is null

jmarin · 2017-06-14T15:49:04Z

Thanks @schbetsy. I would be surprised if that error is related to this PR, maybe it requires a bit more investigation.

schbetsy · 2017-06-14T16:38:07Z

api/src/main/resources/application.conf

+
+    retry-unsuccessful-join-after = 20s
+
+    //DON'T USE IN PRODUCTION!!!!!


What will this value be in production? Why do we need a temporary value here now?

Auto downing of nodes is not recommended in production systems due to the possibility of network failures that form a partition in the cluster. It's useful in development so that the cluster lifecycle (joining, unreachable, leaving, etc. ) can be tested

Okay. Let's make sure there's an open issue in GH to remind us to update that value before we go to production.

Created. #1017

schbetsy · 2017-06-14T16:43:09Z

query/src/main/resources/application.conf

+cassandra {
+  host = "127.0.0.1"
+  host = ${CASSANDRA_CLUSTER_HOSTS}
+  port = 9142


Sometimes the Cassandra port is set to 9142 and sometimes 9042. I haven't figured out the pattern for which should be used where. What's the logic?

This is a mistake from a previous try. CassandraUnit uses 9142, but we're not using it when running the application. Will change back to default port

schbetsy · 2017-06-14T19:26:34Z

query/src/main/scala/hmda/query/repository/institutions/InstitutionCassandraRepository.scala

+
+  def preparedStatement(implicit session: Session): PreparedStatement = {
+    session.prepare(s"INSERT INTO $keyspace.institutions" +
+      s"(id," +


Lines 23-51 don't have substitutions on them, so they probably don't need the s before the ".

schbetsy · 2017-06-14T19:59:01Z

query/src/main/scala/hmda/query/projections/institutions/InstitutionCassandraProjection.scala

+
+class InstitutionCassandraProjection extends InstitutionCassandraRepository {
+
+  def startUp(): Unit = {


I'm not sure I'm understanding this method correctly. It looks like, when the system starts up (in HmdaPlatform.scala), it uses this method to replay all of the Institution-related events from the read journal into Cassandra.

Is that the case? Can you give any more context to help clarify?

This starts up a stream of events from the journal, yes. The difference from previous implementations (i.e. what happens in validation) is that this stream is running continuously, listening to changes in the journal to push those events to the read model. One part that we haven't done yet (because it's not really 100% clear yet what the requirements are) is storing snapshots as an optimization, so not every event has to be read back to the read model.

schbetsy · 2017-06-14T20:34:50Z

query/src/main/scala/hmda/query/repository/institutions/InstitutionCassandraRepository.scala

+    session.execute(query)
+  }
+
+  override def insertData(source: Source[InstitutionQuery, NotUsed]): Future[Done] = {


Looks like the insertData and readData methods are currently used in a spec (InstitutionCassandraRepositorySpec), but they aren't called explicitly from anywhere else in the code. Is this another case where they're called implicitly somewhere?

Not right now. Both of those are convenience methods. Reading will happen when we introduce publication (down the road) and the write method is useful if we need to populate a panel file directly (i.e. for historical data)

schbetsy · 2017-06-15T14:36:42Z

api/src/test/resources/application.conf

+
+cassandra {
+  host = "127.0.0.1"
+  port = 9142


Should this port also change from 9142 to 9042?

schbetsy · 2017-06-15T14:37:11Z

query/src/test/resources/application.conf

+
+cassandra {
+  host = "127.0.0.1"
+  port = 9142


Same question as above about port 9142

No, because EmbeddedCassandraServerHelper uses port 9142 for tests

schbetsy · 2017-06-15T17:52:12Z

I've seen this in the Travis output during the specs, but it doesn't come up when I run the specs locally.

[error] Error during tests:
[error]   Running java with options -classpath /home/travis/build/cfpb/hmda-platform/query/target/scala-2.12/test-classes:/home/travis/build/cfpb/hmda-platform/query/target/scala-2.12/classes:
           [...  I truncated this really long classpath]
           2.10.6/org.scala-sbt/sbt/0.13.13/test-interface-1.0.jar sbt.ForkMain 38993 failed with exit code 137

This is new as of this PR, right? Any idea where it might be coming from?

schbetsy · 2017-06-15T20:43:29Z

We'll merge this around midday tomorrow, even if Travis doesn't pass. Tests pass locally and smoke tests look good, so we're calling this safe to merge.

schbetsy · 2017-06-16T15:29:22Z

build.sbt

@@ -162,7 +162,8 @@ lazy val query = (project in file("query"))
        oldStrategy(x)
    },
    parallelExecution in Test := false,
-    libraryDependencies ++= configDeps ++ akkaPersistenceDeps ++ slickDeps
+    fork in Test := true,


It looks like this line is causing the java.io.EOFException that causes the "sbt.ForkMain failed with exit code 137" error in Travis.

Is forking necessary in the query project's specs? If so, why?

EDIT: Well, I can't actually confirm that this line is causing the error. But my question still stands.
EDIT2: Anecdotally, I removed fork in Test := true on a test branch, and tests all passed in Travis.

Source: I learned more about this error by reading this GH issue from SBT.

jmarin added 24 commits May 31, 2017 14:01

Add embedded cassandra for tests

a99e779

Add sample CassandraSpec

54ab459

Add Cassandra repository trait

4e1b24c

Add operations to cassandra repository

b9d6a4c

Add read operation

08dad60

Change port

fee593c

Fix typo, implement read test

914f99e

Remove unused import

e7173c8

Update test

a5bc6ae

Test institution cassandra repository

e6e7ebe

Turn test logging off

348c541

Remove unused data file

5f5b241

Add keyspace creation to test

d1cd104

Partial implementation of cassandra projection

4815d0d

Streaming events for institutions projection

60a4d23

Merge branch 'master' into institutions-cassandra

0641d1f

Add retry when creating session

bc33d61

Test update (not working yet)

0753a97

Go back to sync implementation

68a4038

Change session to val (stream won't work otherwise)

94eda17

Update test

b1c63b5

Remove embedded cassandra

158601d

Add retries for cassandra query connection, update docker-compose

fa9a836

Remove println

097b837

jmarin added this to the Sprint 33 milestone Jun 13, 2017

jmarin added 4 commits June 13, 2017 09:30

Merge branch 'master' into institutions-cassandra

549ef30

Update project test dependencies and configuration

d41b8c4

Simplify expression

c9be5ee

Increase cassandra unit timeout

c4b818e

jmarin changed the title ~~WIP: Institutions cassandra~~ Institutions cassandra Jun 13, 2017

Uncomment port setting

6f8177a

schbetsy reviewed Jun 14, 2017

View reviewed changes

Change default cassandra port, remove string interpolation

90e6c74

schbetsy reviewed Jun 15, 2017

View reviewed changes

schbetsy mentioned this pull request Jun 15, 2017

application.conf update for prod: remove auto-down setting #1017

Closed

schbetsy reviewed Jun 16, 2017

View reviewed changes

Do not fork tests

1172811

schbetsy merged commit 0ef4f2a into cfpb:master Jun 16, 2017

jmarin deleted the institutions-cassandra branch June 21, 2017 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Institutions cassandra #1013

Institutions cassandra #1013

jmarin commented Jun 13, 2017 •

edited

codecov-io commented Jun 13, 2017 •

edited

schbetsy commented Jun 14, 2017

jmarin commented Jun 14, 2017

schbetsy Jun 14, 2017

jmarin Jun 15, 2017

schbetsy Jun 15, 2017

schbetsy Jun 15, 2017

schbetsy Jun 14, 2017

jmarin Jun 15, 2017

schbetsy Jun 14, 2017

schbetsy Jun 14, 2017

jmarin Jun 15, 2017

schbetsy Jun 14, 2017

jmarin Jun 15, 2017

schbetsy Jun 15, 2017

schbetsy Jun 15, 2017

jmarin Jun 15, 2017

schbetsy commented Jun 15, 2017

schbetsy commented Jun 15, 2017

schbetsy Jun 16, 2017 •

edited

schbetsy Jun 16, 2017


		retry-unsuccessful-join-after = 20s

		//DON'T USE IN PRODUCTION!!!!!


		class InstitutionCassandraProjection extends InstitutionCassandraRepository {

		def startUp(): Unit = {

Institutions cassandra #1013

Institutions cassandra #1013

Conversation

jmarin commented Jun 13, 2017 • edited

codecov-io commented Jun 13, 2017 • edited

Codecov Report

schbetsy commented Jun 14, 2017

jmarin commented Jun 14, 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

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

schbetsy commented Jun 15, 2017

schbetsy commented Jun 15, 2017

schbetsy Jun 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarin commented Jun 13, 2017 •

edited

codecov-io commented Jun 13, 2017 •

edited

schbetsy Jun 16, 2017 •

edited