[
https://issues.apache.org/jira/browse/SAMZA-86?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13830268#comment-13830268
]
Jakob Homan commented on SAMZA-86:
----------------------------------
Since I like code to be as clear as possible about intent, I am forced to
prefer the Map[String, Option[String]], as it is the most clear as to what the
intent of the code:
{code}scala> var y = Map[String,Option[String]]("a" -> Some("A"), "b" -> None)
y: scala.collection.immutable.Map[String,Option[String]] = Map((a,Some(A)),
(b,None))
scala> y.get("a")
res5: Option[Option[String]] = Some(Some(A))
scala> y.get("a").get.get
res6: String = A
{code}
By the third get, I think it's abundantly clear what we're trying to do.
More realistically, doing the null to Option conversion at the
addTopicPartition call is probably the best choice. I'd prefer to have the map
contain an Option and None to be equivalent to what we have now, but Scala just
does not want to collapse that option in any reasonable way.
It would be good to be consistent, or at least loud, about where this type of
conversion is done:
{noformat}
[jhoman@jhoman-ld incubator-samza]$ ack "[^:] Option(.*) match"
samza-yarn/src/main/scala/org/apache/samza/job/yarn/YarnJob.scala
68: Option(getStatus) match {
83: Option(getStatus) match {
samza-kafka/src/main/scala/org/apache/samza/system/kafka/GetOffset.scala
86: val actualOffset = Option(lastCheckpointedOffset) match {
samza-core/src/main/scala/org/apache/samza/metrics/JvmMetrics.scala
125: Option(threadInfo) match {
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala
105: Option(job.waitForStatus(Running, 500)) match {
{noformat}
For each of these, it would be good to explicitly call out the conversion,
rather than wrapping in option. That can be done in a different JIRA.
> GetOffset:getNextOffset needs some work
> ---------------------------------------
>
> Key: SAMZA-86
> URL: https://issues.apache.org/jira/browse/SAMZA-86
> Project: Samza
> Issue Type: Improvement
> Reporter: Jakob Homan
> Assignee: Rekha Joshi
> Attachments: SAMZA_86_1.patch
>
>
> {code} def getNextOffset(sc: SimpleConsumer with DefaultFetch, tp:
> TopicAndPartition, lastCheckpointedOffset: String): Long = {
> val offsetRequest = new OffsetRequest(Map(tp -> new
> PartitionOffsetRequestInfo(getAutoOffset(tp.topic), 1)))
> val offsetResponse = sc.getOffsetsBefore(offsetRequest)
> val partitionOffsetResponse =
> offsetResponse.partitionErrorAndOffsets.get(tp).getOrElse(toss("Unable to
> find offset information for %s" format tp))
> val autoOffset =
> partitionOffsetResponse.offsets.headOption.getOrElse(toss("Got response, but
> no offsets defined for %s" format tp))
> info("Got offset %d for topic and partition %s" format (autoOffset, tp))
> val actualOffset = Option(lastCheckpointedOffset) match {
> case Some(last) => useLastCheckpointedOffset(sc, last,
> tp).getOrElse(autoOffset)
> case None => autoOffset
> }
> {code}
> lastCheckpointedOffset being coerced into an option here sucks, particularly
> since at least one QA job is passing in a null as the value, which hits the
> Some case. It would be better to make the parameter an Option at the method
> level and be explicit about what needs to be passed in.
--
This message was sent by Atlassian JIRA
(v6.1#6144)
