Release/r97 knossos #3502

knservis · 2017-11-15T10:36:30Z

Prepare for release:

Update changelog
Update version
Update readme with travis icon
Tests depend on SCE release (fix before merging https://travis-ci.org/snowplow/snowplow/pull_requests)
Depends on igc release: R82 - supporting snowplow r97 knossos iglu-central#664

BenFradet

hey @knservis i did a first pass, here's my feedback:

you need to bump spark enrich
check the correspondance between the changelog, the commits, the issues
you need to publish qa artifacts for sce (0.28.0-M1) and spark enrich (1.11.0-rc1)
be a bit more mindful of code hygiene

👍

BenFradet · 2017-11-27T16:03:45Z

...in/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/MailgunAdapter.scala

+                      )).success
+                    }
+                  } 
+                }


can't we refactor those 2 nested failure/success into a for comprehension?

BenFradet · 2017-11-27T16:04:09Z

...in/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/MailgunAdapter.scala

+          bodyMap.get("event") match {
+            case None            => s"No ${VendorName} event parameter provided: cannot determine event type".failNel
+            case Some(eventType) => {
+              (payloadBodyToEvent(bodyMap)) match {


unnecessary parens

BenFradet · 2017-11-27T16:05:13Z

...in/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/MailgunAdapter.scala

+        try {
+          val bodyMap = getBoundary(ct)
+                          .map(parseMultipartForm(body, _))
+                          .getOrElse(toMap(URLEncodedUtils.parse(URI.create("http://localhost/?" + body), "UTF-8").toList))


if we want to be consistent with other parts of the codebase this will look like:

val bodyMap = getBoundary(ct) .map(parseMultipartForm(body, _)) .getOrElse(toMap(URLEncodedUtils.parse(URI.create("http://localhost/?" + body), "UTF-8").toList))

BenFradet · 2017-11-27T16:12:08Z

...in/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/MailgunAdapter.scala

+  private def parseMultipartForm(body: String, boundary: String): Map[String, String] =
+    body
+      .split(s"--$boundary")
+      .flatMap( _ match {


no need for _ match {

BenFradet · 2017-11-27T16:12:52Z

...in/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/MailgunAdapter.scala

+   * Content-Disposition: form-data; anything="notllokingintothis"; name="key"
+   *
+   * value
+   *


empty lines

Actually the empty line above "value" is mandated by the RFC mentioned.

ah my bad I thought it was some weird comment formatting

BenFradet · 2017-11-27T16:24:47Z

...main/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/OlarkAdapter.scala

+            case NonFatal(e) => {
+              val exception = JU.stripInstanceEtc(e.toString).orNull
+              s"${VendorName} could not parse body: [${exception}]".failNel
+            }


pls try to limit brackets here and in the other files too

BenFradet · 2017-11-27T16:25:39Z

...cala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/StatusGatorAdapter.scala

+              val exception = JU.stripInstanceEtc(e.toString).orNull
+              s"${VendorName} event string failed to parse into JSON: [${exception}]".failNel
+            }
+            case e: Exception => {


BenFradet · 2017-11-27T16:27:33Z

...cala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/StatusGatorAdapter.scala

+          s"${VendorName} event body is empty: nothing to process".failNel
+        } else {
+          val qsParams = toMap(payload.querystring)
+          try {


here and for the others too, can't we move the try/catch closer to what can actually throw an exception, I'm guessing the parse?

Also can't this logic be factored out? I feel like I'm seeing the same logic in all the adapters

BenFradet · 2017-11-27T16:28:03Z

...n/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/UnbounceAdapter.scala

+  // Schema for Unbounce event context
+  private val ContextSchema = Map(
+    "event_context" -> SchemaKey("com.unbounce", "event_context", "jsonschema", "1-0-0").toSchemaUri
+  )


do we need a map here?

BenFradet · 2017-11-27T16:28:19Z

...n/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/UnbounceAdapter.scala

+          try {
+            val bodyMap = toMap(URLEncodedUtils.parse(URI.create("http://localhost/?" + body), "UTF-8").toList)
+            payloadBodyToContext(bodyMap) match {
+              case Success(validatedBodyMap) =>               {


empty spaces

…afe (closes #3481)

BenFradet · 2017-12-06T14:54:32Z

...in/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/MailgunAdapter.scala

+                }
+              }
+            }
+          }


can't we refactor those nested matches as:

// ... case Some(eventType) => for { event <- payloadBodyToEvent(bodyMap).leftMap(_.wrapNel) schemaUri <- lookupSchema(eventType.some, VendorName, EventSchemaMap) } yield NonEmptyList(RawEvent( api = payload.api, parameters = toUnstructEventParams(TrackerVersion, params, schemaUri, cleanupJsonEventValues( mutateMailgunEvent(event), ("event", eventType).some, "timestamp"), "srv"), contentType = payload.contentType, source = payload.source, context = payload.context ))

this will be way more readable imo

BenFradet · 2017-12-06T14:55:23Z

...in/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/MailgunAdapter.scala

+      case (Some(body), _) if (body.isEmpty)                        => s"${VendorName} event body is empty: nothing to process".failNel
+      case (Some(body), Some(ct))                                   => {
+        val params = toMap(payload.querystring)
+        try {


can't we take the try/catch closer to what can actually throw?

BenFradet · 2017-12-06T14:56:08Z

...main/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/OlarkAdapter.scala

+          }
+        } catch {
+          case NonFatal(e) => s"${VendorName} could not parse body: [${JU.stripInstanceEtc(e.getMessage).orNull}]".failNel
+        }


same as before:

for comp

try/catch closer to what can throw

BenFradet · 2017-12-06T14:58:09Z

...main/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/OlarkAdapter.scala

+   *
+   * @param oTs olark timestamp string
+   * @return Long msec
+   * @throws MatchError and NumberFormatException


pls handle those at the lowest level possible, make this an option or a Validation

It shouldn't be the caller's responsibility to blindly handle a function's shortcomings.

BenFradet · 2017-12-08T15:30:06Z

...main/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/OlarkAdapter.scala

+            val exception = JU.stripInstanceEtc(e.getMessage).orNull
+            s"${VendorName} incorrect event string : [${exception}]".fail
+          }
+>>>>>>> 066c7f20... Spark Enrich: add test for Olark Adapter (closes #2792)


… service (closes #3503)

…rm that doesn't have native-lzo (closes #3508)

alexanderdean · 2017-12-13T18:15:14Z

Great! Next step is to put the rc live on the standard canaries.

alexanderdean · 2017-12-13T18:20:50Z

Awesome, UA is the next step - getting the rc onto our canaries. @BenFradet let me know if you need any help with that.

alexanderdean

UA has gone fine on snplow2, so green tick from me.

#3525)

…#3529)

…ptor (closes #3538)

…1.0 (closes #3002)

closes #3516)

BenFradet · 2017-12-18T11:39:54Z

@knservis Could you rebase so that the the "Prepare for release" is last and remove rc commits?

knservis · 2017-12-18T11:42:54Z

@BenFradet done

BenFradet · 2017-12-18T11:57:31Z

@knservis the date of the "Prepared for release" commit needs to be updated otherwise it's not chronological

…to release/r9x-knossos

BenFradet · 2017-12-18T15:13:03Z

Merged, closing

knservis requested review from alexanderdean, chuwy and BenFradet November 15, 2017 10:36

knservis force-pushed the release/r9x-knossos branch 5 times, most recently from a442479 to 880db56 Compare November 16, 2017 21:11

knservis changed the title ~~Release/r9x knossos~~ Release/r97 knossos Nov 27, 2017

knservis force-pushed the release/r9x-knossos branch from d4f9fd7 to 0f79268 Compare November 27, 2017 11:40

alexanderdean removed the request for review from chuwy November 27, 2017 13:21

BenFradet reviewed Nov 27, 2017

View reviewed changes

knservis force-pushed the release/r9x-knossos branch 3 times, most recently from ad68f67 to 3ab93eb Compare November 30, 2017 15:57

knservis added 2 commits December 5, 2017 11:10

Scala Common Enrich: bump user-agent-utils to 1.20 (closes #2930)

ca648dd

Scala Common Enrich: make enrichments.ExtractEventTypeSpec timezone-s…

f83d8f6

…afe (closes #3481)

knservis force-pushed the release/r9x-knossos branch from 3ab93eb to 7284d64 Compare December 5, 2017 09:36

BenFradet assigned knservis Dec 5, 2017

knservis force-pushed the release/r9x-knossos branch 6 times, most recently from fe49298 to 0fcbb97 Compare December 6, 2017 14:25

BenFradet reviewed Dec 6, 2017

View reviewed changes

knservis force-pushed the release/r9x-knossos branch 3 times, most recently from bc4720b to aa02422 Compare December 7, 2017 18:24

BenFradet reviewed Dec 8, 2017

View reviewed changes

knservis added 3 commits December 13, 2017 19:33

Common: rename AWS deployment credentials in .travis.yml (close #3115)

1bfaa72

Spark Enrich: fix tests that fail when running on an alternative iglu…

bc0493e

… service (closes #3503)

Spark Enrich: fix tests that fail with error when running on a platfo…

df2808b

…rm that doesn't have native-lzo (closes #3508)

knservis force-pushed the release/r9x-knossos branch 2 times, most recently from 6ad7e57 to b1d21ea Compare December 13, 2017 17:43

knservis force-pushed the release/r9x-knossos branch 2 times, most recently from 46d8c00 to 58cf7e2 Compare December 14, 2017 16:51

alexanderdean approved these changes Dec 16, 2017

View reviewed changes

knservis and others added 9 commits December 18, 2017 11:14

Spark Enrich: bump to 1.11.0 (closes #3533)

a55d4e9

Scala Common Enrich: remove toSecond parameter in Adapter (closes #3534)

bfd7c5a

EmrEtlRunner: uncompress gzipped raw files when copying to HDFS (closes

9ba57e4

#3525)

EmrEtlRunner: bump to 0.30.0 (closes #3526)

552ae40

EmrEtlRunner: add ability to skip RDB Loader consistency check (closes …

d749551

…#3529)

Scala Common Enrich: add function to camelCase all JSON fields in Ada…

0f6d370

…ptor (closes #3538)

EmrEtlRunner: update spark_enrich version in config.yml.sample to 1.1…

dbdf5d0

…1.0 (closes #3002)

Common: reenable publishLocal in travis for spark enrich tests to pass (

3c606fd

closes #3516)

Prepare for release

d39cf8e

knservis force-pushed the release/r9x-knossos branch from 58cf7e2 to 3b1f0dd Compare December 18, 2017 09:14

knservis force-pushed the release/r9x-knossos branch from 3b1f0dd to d39cf8e Compare December 18, 2017 11:42

Prepare for release

98b79d4

BenFradet force-pushed the release/r9x-knossos branch from d39cf8e to 98b79d4 Compare December 18, 2017 12:51

Merge branch 'release/r9x-knossos' of github.com:snowplow/snowplow in…

df8a233

…to release/r9x-knossos

BenFradet closed this Dec 18, 2017

BenFradet deleted the release/r9x-knossos branch December 18, 2017 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release/r97 knossos #3502

Release/r97 knossos #3502

knservis commented Nov 15, 2017 •

edited by alexanderdean

BenFradet left a comment

BenFradet Nov 27, 2017

BenFradet Nov 27, 2017

BenFradet Nov 27, 2017

BenFradet Nov 27, 2017

BenFradet Nov 27, 2017

knservis Dec 4, 2017

BenFradet Dec 4, 2017

BenFradet Nov 27, 2017

BenFradet Nov 27, 2017

BenFradet Nov 27, 2017

BenFradet Nov 27, 2017

BenFradet Nov 27, 2017

BenFradet Dec 6, 2017

BenFradet Dec 6, 2017

BenFradet Dec 6, 2017

BenFradet Dec 6, 2017

BenFradet Dec 8, 2017

alexanderdean commented Dec 13, 2017

alexanderdean commented Dec 13, 2017

alexanderdean left a comment

BenFradet commented Dec 18, 2017

knservis commented Dec 18, 2017

BenFradet commented Dec 18, 2017

BenFradet commented Dec 18, 2017

Release/r97 knossos #3502

Release/r97 knossos #3502

Conversation

knservis commented Nov 15, 2017 • edited by alexanderdean

BenFradet 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

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

alexanderdean commented Dec 13, 2017

alexanderdean commented Dec 13, 2017

alexanderdean left a comment

Choose a reason for hiding this comment

BenFradet commented Dec 18, 2017

knservis commented Dec 18, 2017

BenFradet commented Dec 18, 2017

BenFradet commented Dec 18, 2017

knservis commented Nov 15, 2017 •

edited by alexanderdean