Scalaapi #2673

skanjila · 2018-01-20T23:37:01Z

Added the first set of scala files and bazel build file that links in java implementations.

move scripts/docker to scripts/images (#2672) * build docker images in build-artifacts.sh * rename scripts/docker to scripts/images for building docker images added all new scala files from converter

…laapi

CLAassistant · 2018-01-20T23:37:08Z

All committers have signed the CLA.

erenavsarogullari · 2018-01-21T16:26:14Z

heron/api/src/scala/BUILD

@@ -0,0 +1,52 @@
+licenses(["notice"])


General BUILD Comment:
*.scala files are not referenced here. Bazel supports Scala via
https://github.com/bazelbuild/rules_scala
so i think we need to use scala_library to compile scala files by referencing
com/twitter/heron/streamlet/scala/**/*.scala as src.

erenavsarogullari · 2018-01-21T16:31:13Z

heron/api/src/scala/Builder.scala

@@ -0,0 +1,46 @@
+//


General Comment:
Currently, All files look living under heron/api/src/scala.

Related *.scala files should be under package: heron/api/src/scala/com/twitter/heron/streamlet/scala (except BUILD file)
e.g: Currently, Java Streamlet API lives under heron/api/src/java/com/twitter/heron/streamlet

erenavsarogullari · 2018-01-21T16:35:02Z

heron/api/src/scala/Builder.scala

+    * All sources of the computation should register using addSource.
+    * @param supplier The supplier function that is used to create the streamlet
+    */
+  def newSource[R](supplier: SerializableSupplier[R]): Streamlet[R]


In the light of Scala User perspective and in my opinion, All exposed Scala Traits should be pure scala. This means Scala traits should accept Scala Functions instead of FunctionalInterface here:

def newSource[R](supplier: () => R): Streamlet[R]

instead of

def newSource[R](supplier: SerializableSupplier[R]): Streamlet[R]

erenavsarogullari · 2018-01-21T16:39:49Z

heron/api/src/scala/SerializableBiFunction.scala

@@ -0,0 +1,28 @@
+//


General Serializable-FunctionalInterfaces Comment
I think all Serializable Functional Interfaces(SerializableFunction, SerializableConsumer etc...) should be hidden from Scala Users. This means Scala API should just accept pure Scala functions instead of them. In the light of this, i think we do not need to have these Serializable interfaces' s Scala versions. These can be wrapped at Scala StreamletImpl level by transforming incoming Scala Function to related SerializableFunctionalInterface

erenavsarogullari · 2018-01-21T16:41:59Z

heron/api/src/scala/Streamlet.scala

+    * Return a new Streamlet by applying mapFn to each element of this Streamlet
+    * @param mapFn The Map Function that should be applied to each element
+    */
+  def map[T](mapFn: SerializableFunction[R, _ <: T]): Streamlet[T]


In the light of my previous comment, as a sample for map function, i think this should be as follows.
def map[T](mapFn: R => _ <: T): Streamlet[T]
instead of
def map[T](mapFn: SerializableFunction[R, _ <: T]): Streamlet[T]

Same comment for the following functions.

…ugin

move scripts/docker to scripts/images (#2672) * build docker images in build-artifacts.sh * rename scripts/docker to scripts/images for building docker images * added scala build files and bazel file move scripts/docker to scripts/images (#2672) * build docker images in build-artifacts.sh * rename scripts/docker to scripts/images for building docker images added all new scala files from converter added all new scala files from converter again added all new scala files from converter added all new scala files from converter again code reorganization and added some bits to build file to use scala plugin

…laapi

nwangtw · 2018-01-22T18:55:59Z

heron/api/src/scala/com/twitter/heron/streamlet/Config.scala

+  * user-defined key/value pairs are passed on to the topology runner via
+  * this class.
+  */
+@SerialVersionUID(6204498077403076352L)


UIDs seem to be the same as Java version. Just to confirm if this what we want or not.

nwangtw · 2018-01-23T17:51:28Z

heron/api/src/scala/com/twitter/heron/streamlet/Config.scala

+  def newBuilder(): Builder = new Builder()
+
+  private def translateSemantics(semantics: DeliverySemantics)
+  : com.twitter.heron.api.Config.TopologyReliabilityMode = semantics match {


consider import the package as
import com.twitter.heron.api.{Config => ApiConfig}

then use ApiConfig.TopologyReliabilityMode, etc

instead of full path.

nwangtw · 2018-01-23T17:56:31Z

heron/api/src/scala/com/twitter/heron/streamlet/KeyValue.scala

+  * serialized between components.
+  */
+@SerialVersionUID(-7120757965590727554L)
+class KeyValue[K, V]() extends Serializable {


I am wondering if we can use case class here? if it is not expected to be extended.

case class KeyValue[K, V](
@BeanProperty val key: K,
@BeanProperty val value V) {
override def toString() = ....
}

val or var depending on the usage.

nwangtw · 2018-01-23T17:57:12Z

heron/api/src/scala/com/twitter/heron/streamlet/KeyedWindow.scala

+  * using this class
+  */
+@SerialVersionUID(4193319775040181971L)
+class KeyedWindow[T]() extends Serializable {


same as KeyValue, maybe case class if possible.

nwangtw · 2018-01-23T17:58:15Z

heron/api/src/scala/com/twitter/heron/streamlet/Runner.scala

+  def run(name: String, config: Config, builder: Builder): Unit = {
+    val bldr: BuilderImpl = builder.asInstanceOf[BuilderImpl]
+    val topologyBuilder: TopologyBuilder = bldr.build()
+    try HeronSubmitter.submitTopology(name,


suggest to add {}.

try {
...
} catch {
...
}

nwangtw · 2018-01-23T17:59:16Z

heron/api/src/scala/com/twitter/heron/streamlet/Runner.scala

+    * @param builder The builder used to keep track of the sources.
+    */
+  def run(name: String, config: Config, builder: Builder): Unit = {
+    val bldr: BuilderImpl = builder.asInstanceOf[BuilderImpl]


is this line necessary?

nwangtw · 2018-01-23T18:23:23Z

heron/api/src/scala/com/twitter/heron/streamlet/Window.scala

+  * inside their streamlets using this container.
+  */
+@SerialVersionUID(5103471810104775854L)
+class Window() extends Serializable {


another candidate to be case class.

sijie · 2018-01-23T19:31:07Z

heron/api/src/scala/com/twitter/heron/streamlet/KeyValue.scala

+  }
+
+  override def toString(): String =
+    "{ " + String.valueOf(key) + " : " + String.valueOf(value) +


can't we just use "{ " + key + " : " + value + " }"?

sijie · 2018-01-23T19:31:42Z

heron/api/src/scala/com/twitter/heron/streamlet/KeyedWindow.scala

+  }
+
+  override def toString(): String =
+    "{ Key: " + String.valueOf(key) + " Window: " + window.toString +


do we really need String.valueOf or toString? if you just "{ key: " + key + " window: " + window + " }", it is better than explicitly call toString, because it handles null field, while window.toString would throw NPE if window is null.

sijie · 2018-01-23T19:36:59Z

heron/api/src/scala/com/twitter/heron/streamlet/Streamlet.scala

+    * Return a new Streamlet by applying mapFn to each element of this Streamlet
+    * @param mapFn The Map Function that should be applied to each element
+    */
+  def map[T](mapFn: SerializableFunction[R, _ <: T]): Streamlet[T]


SerializableFunction is a java function, not a scala function. which I think you can't really use all those scala syntactic sugar. if we are generating a scala oriented api, it would be better to think about taking scala function, rather than using java functions.

skanjila and others added 5 commits January 20, 2018 15:31

added all new scala files from converter

1e18dc1

added all new scala files from converter again

60529cd

* added scala build files and bazel file

15570f4

move scripts/docker to scripts/images (#2672) * build docker images in build-artifacts.sh * rename scripts/docker to scripts/images for building docker images added all new scala files from converter

added all new scala files from converter again

25890fd

Merge branch 'scalaapi' of https://github.com/skanjila/heron into sca…

9cbe908

…laapi

erenavsarogullari reviewed Jan 21, 2018

View reviewed changes

skanjila and others added 5 commits January 21, 2018 09:53

code reorganization and added some bits to build file to use scala pl…

14a6fe9

…ugin

Code reorg

e5a652f

Code reorg

115e035

Merge branch 'scalaapi' of https://github.com/skanjila/heron into sca…

b762db3

…laapi

nwangtw reviewed Jan 22, 2018

View reviewed changes

nwangtw reviewed Jan 23, 2018

View reviewed changes

sijie reviewed Jan 23, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scalaapi #2673

Scalaapi #2673

skanjila commented Jan 20, 2018

CLAassistant commented Jan 20, 2018 •

edited

Loading

erenavsarogullari Jan 21, 2018

erenavsarogullari Jan 21, 2018

erenavsarogullari Jan 21, 2018

erenavsarogullari Jan 21, 2018

erenavsarogullari Jan 21, 2018

nwangtw Jan 22, 2018

nwangtw Jan 23, 2018

nwangtw Jan 23, 2018

nwangtw Jan 23, 2018

nwangtw Jan 23, 2018

nwangtw Jan 23, 2018

nwangtw Jan 23, 2018

sijie Jan 23, 2018

sijie Jan 23, 2018

sijie Jan 23, 2018

Scalaapi #2673

Are you sure you want to change the base?

Scalaapi #2673

Conversation

skanjila commented Jan 20, 2018

CLAassistant commented Jan 20, 2018 • 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

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

CLAassistant commented Jan 20, 2018 •

edited

Loading