comeara / pillar

Pillar manages migrations for your Cassandra data stores.
https://github.com/comeara/pillar
MIT License
111 stars 64 forks source link

authored_at timestamp is set to a long time in the past during migration #21

Closed rdawemsys closed 9 years ago

rdawemsys commented 9 years ago

I have been playing with Pillar for Cassandra schema migrations. I noticed that the applied_migrations.authored_at column is not set up correctly.

For instance, my migration CQL files have the following authoredAt markup:

rdawe@cstar:~/MO-3530/test-pillar$ grep -i authoredat conf/pillar/migrations/mydata/*
conf/pillar/migrations/mydata/1420779600_create_test.cql:-- authoredAt: 1420779600
conf/pillar/migrations/mydata/1420783200_add_column_test.cql:-- authoredAt: 1420783200
rdawe@cstar:~/MO-3530/test-pillar$ perl -e 'print gmtime(1420779600)."\n";'
Fri Jan  9 05:00:00 2015
rdawe@cstar:~/MO-3530/test-pillar$ perl -e 'print gmtime(1420783200)."\n";'
Fri Jan  9 06:00:00 2015

and this results in the following in the applied migrations table:

rdawe@cstar:~/MO-3530/test-pillar$ cqlsh
Connected to Test Cluster at localhost:9160.
[cqlsh 4.1.1 | Cassandra 2.0.8 | CQL spec 3.1.1 | Thrift protocol 19.39.0]
Use HELP for help.
cqlsh> USE test;
cqlsh:test> SELECT * from applied_migrations ;

 authored_at              | description                 | applied_at
--------------------------+-----------------------------+--------------------------
 1970-01-17 05:39:43-0500 | add column to example table | 2015-01-13 08:30:45-0500
 1970-01-17 05:39:39-0500 |           create test table | 2015-01-13 08:30:44-0500

(2 rows)
rdawemsys commented 9 years ago

The issue seems to be that authored_at is not converted to milliseconds before being converted to a Date.

Suggested patch below. Please note that the test case fails after this update -- I'm not a Scala programmer, so I wasn't sure how to fix the test.

diff --git a/src/main/scala/com/chrisomeara/pillar/Parser.scala b/src/main/scala/com/chrisomeara/pillar/Parser.scala
index 4f7a550..de196b4 100644
--- a/src/main/scala/com/chrisomeara/pillar/Parser.scala
+++ b/src/main/scala/com/chrisomeara/pillar/Parser.scala
@@ -21,15 +21,15 @@ class PartialMigration {

     if (description.isEmpty) errors("description") = "must be present"
     if (authoredAt.isEmpty) errors("authoredAt") = "must be present"
-    if (!authoredAt.isEmpty && authoredAtAsLong < 1) errors("authoredAt") = "must be a number greater than zero"
+    if (!authoredAt.isEmpty && authoredAtAsMillis < 1) errors("authoredAt") = "must be a number greater than zero"
     if (up.isEmpty) errors("up") = "must be present"

     if (!errors.isEmpty) Some(errors.toMap) else None
   }

-  def authoredAtAsLong: Long = {
+  def authoredAtAsMillis: Long = {
     try {
-      authoredAt.toLong
+      authoredAt.toLong * 1000
     } catch {
       case _:NumberFormatException => -1
     }
@@ -80,11 +80,11 @@ class Parser {
         inProgress.down match {
           case Some(downLines) =>
             if (downLines.isEmpty) {
-              Migration(inProgress.description, new Date(inProgress.authoredAtAsLong), inProgress.up.mkString("\n"), None)
+              Migration(inProgress.description, new Date(inProgress.authoredAtAsMillis), inProgress.up.mkString("\n"), None)
             } else {
-              Migration(inProgress.description, new Date(inProgress.authoredAtAsLong), inProgress.up.mkString("\n"), Some(downLines.mkString("\n")))
+              Migration(inProgress.description, new Date(inProgress.authoredAtAsMillis), inProgress.up.mkString("\n"), Some(downLines.mkString("\n")))
             }
-          case None => Migration(inProgress.description, new Date(inProgress.authoredAtAsLong), inProgress.up.mkString("\n"))
+          case None => Migration(inProgress.description, new Date(inProgress.authoredAtAsMillis), inProgress.up.mkString("\n"))
         }
     }
   }
diff --git a/src/test/scala/com/chrisomeara/pillar/ParserSpec.scala b/src/test/scala/com/chrisomeara/pillar/ParserSpec.scala
index 90679f8..b1daa6c 100644
--- a/src/test/scala/com/chrisomeara/pillar/ParserSpec.scala
+++ b/src/test/scala/com/chrisomeara/pillar/ParserSpec.scala
@@ -17,7 +17,8 @@ class ParserSpec extends FunSpec with BeforeAndAfter with ShouldMatchers {

       it("assigns authoredAt") {
         val resource = new FileInputStream(migrationPath)
-        Parser().parse(resource).authoredAt should equal(new Date(1370023262))
+        val expected = new Date(1370023262000L).toString
+        Parser().parse(resource).authoredAt should equal(expected)
       }

       it("assigns description") {
@@ -101,4 +102,4 @@ class ParserSpec extends FunSpec with BeforeAndAfter with ShouldMatchers {
       }
     }
   }
-}
\ No newline at end of file
+}
rdawemsys commented 9 years ago

This patch is also needed so that -t works, after my changes above:

diff --git a/src/main/scala/com/chrisomeara/pillar/cli/CommandLineConfiguration.scala b/src/main/scala/com/chrisomeara/pillar/cli/CommandLineConfiguration.scala
index d074aba..709d49d 100644
--- a/src/main/scala/com/chrisomeara/pillar/cli/CommandLineConfiguration.scala
+++ b/src/main/scala/com/chrisomeara/pillar/cli/CommandLineConfiguration.scala
@@ -24,7 +24,10 @@ object CommandLineConfiguration {
         directory
     }
     val environmentOption = parser.option[String](List("e", "environment"), "env", "environment")
-    val timeStampOption = parser.option[Long](List("t", "time-stamp"), "time", "The migration time stamp")
+    val timeStampOption = parser.option[Long](List("t", "time-stamp"), "time", "The migration time stamp") {
+      (timeStamp, _) =>
+        java.lang.Long.parseLong(timeStamp) * 1000
+    }

     parser.parse(arguments)
PeterLappo commented 9 years ago

Hi, The implementation is correct perl -e 'print gmtime(1420779600)."\n";' takes a time in seconds but java.util.date takes milliseconds so authoredAt 1420779600

scala> val x = new java.util.Date(1420779600)
x: java.util.Date = Sat Jan 17 11:39:39 GMT 1970

so just add three zeros to your timestamp

scala> val x = new java.util.Date(1420779600000L)
x: java.util.Date = Fri Jan 09 05:00:00 GMT 2015

By the way I've created a fork of this project that merges in the outstanding pull requests on a branch called patch and also adds an authoredAtDate field where you can specify your date as 2015-01-09 05:00:00, using timestamp was becoming annoying. Version number was bumped to 2.0.2 see https://github.com/smr-co-uk/pillar/tree/patch Peter

rdawemsys commented 9 years ago

Are you saying that the authoredAt timestamp in the migration file is supposed to be in milliseconds?

None of the examples of authoredAt in the documentation are in milliseconds.

authoredAtDate sounds like a useful addition. :)

Thanks, Rich

PeterLappo commented 9 years ago

Yes.

I know, its not ideal, but doesn’t make any difference in practice though, but it makes it easier for humans to understand.

Yes, much easier than trying to figure out a millisecond timestamp if you want to know what date it was authored on.

Peter

www.smr.co.uk

On 4 Mar 2015, at 15:04, Richard Dawe notifications@github.com wrote:

Are you saying that the authoredAt timestamp in the migration file is supposed to be in milliseconds?

None of the examples of authoredAt in the documentation are in milliseconds.

authoredAtDate sounds like a useful addition. :)

Thanks, Rich

— Reply to this email directly or view it on GitHub https://github.com/comeara/pillar/issues/21#issuecomment-77173387.

rdawemsys commented 9 years ago

I'll open a ticket to fix up the documentation instead. Thanks!