[FLINK-2613] Print usage information for Scala Shell #1106

nikste · 2015-09-08T13:11:23Z

changed startup of scala shell

StephanEwen · 2015-09-09T10:10:08Z

Looks good. Is it possible that we cover both cases with tests?

Start the shell in local mode, submit some commands.
Start a ForkableFlinkMiniCluster, and start the shell in mode remote "localhost" cluster.getJobManagerRpcPort()?

fhueske · 2015-09-15T00:19:18Z

The tests use the shell scripts (.sh or .bat) to start the Scala Shell and rely on finding these scripts in the flink-dist/target folder. I think this is a bit fragile. Why not directly using the FlinkShell.scala class?

fhueske · 2015-09-16T16:31:58Z

...g/flink-scala-shell/src/test/scala/org/apache/flink/api/scala/ScalaShellLocalStartupIT.scala

+
+
+/**
+ * Created by nikste on 9/16/15.


Please remove this comment

nikste · 2015-09-17T12:35:11Z

Should be fixed according to your comments @fhueske

fhueske · 2015-09-17T15:56:21Z

...-staging/flink-scala-shell/src/test/scala/org/apache/flink/api/scala/ScalaShellITSuite.scala

+   */
+  test("start flink scala shell with remote cluster") {
+
+    val input: String = "val els = env.fromElements(\"a\",\"b\");\n" +


Can you also add a check, that the program was actually executed for example by checking that the correct output was produced?

fhueske · 2015-09-17T15:57:21Z

Thanks for the update @nikste! I have only one minor thing to add.
After that it's good to merge, IMO.
Thanks!

rmetzger · 2015-09-18T09:37:51Z

@nikste, since you need to change the pull request again, can you also fix the directory name of the classes
flink-staging/flink-scala-shell/src/main/scala/org.apache.flink/api/scala/FlinkShell.scala

nikste · 2015-09-21T08:40:55Z

@fhueske, good point to check if the program was actually executed! @rmetzger fixed the directory structure as well.

chiwanpark · 2015-09-25T17:40:54Z

@nikste Could you renaming the directory by using git mv command? Currently, FlinkShell.scala loses all commit histories. FlinkILoop.scala seems okay.

nikste · 2015-09-30T09:28:05Z

@chiwanpark I moved the files with git mv, however they still don't show the history.
This seems to be more of a gui thing of github according to:

http://stackoverflow.com/questions/31304441/i-moved-all-my-project-files-with-git-mv-but-i-have-lost-all-my-history-why

However, the history still exists and can be accessed via:

git log --follow <some path to a moved file>

sachingoel0101 · 2015-09-30T09:31:05Z

Hey @nikste , you should rebase this to the current master.

chiwanpark · 2015-10-08T11:53:15Z

flink-staging/flink-scala-shell/src/main/scala/org/apache/flink/api/scala/FlinkShell.scala

@@ -68,38 +94,62 @@ object FlinkShell {

  def startShell(
      userHost : String, 
-      userPort : Int, 
+      userPort : Int,
+      executionMode : ExecutionMode.Value,
      externalJars : Option[Array[String]] = None): Unit ={


Please remove space after parameter name like following:

def startShell( userHost: String, userPort: Int, executionMode: ExecutionMode.value

chiwanpark · 2015-10-08T12:13:47Z

Hi @nikste, thanks for update. It looks good to merge except some style issues.

nikste · 2015-10-08T12:19:22Z

@chiwanpark, white line issues are now fixed.

chiwanpark · 2015-10-08T12:57:19Z

Okay, if there is no objection and the travis passes, I'll merge this.

chiwanpark · 2015-10-08T14:16:23Z

flink-staging/flink-scala-shell/src/main/scala/org/apache/flink/api/scala/FlinkShell.scala

+          miniCluster.start()
+          val port = miniCluster.getLeaderRPCPort
+          println(s"\nStarting local Flink cluster (host: localhost, port: $port).\n")
+          ("localhost",port, Some(miniCluster))


Need a space after ,.

chiwanpark · 2015-10-08T14:21:38Z

@nikste Sorry for bothering you, but there are still some style issues. Looks good to merge except the issues.

chiwanpark · 2015-10-08T14:29:22Z

...-staging/flink-scala-shell/src/test/scala/org/apache/flink/api/scala/ScalaShellITSuite.scala

+    // buffered reader for testing
+    FlinkShell.bufferedReader = Some(in)
+    FlinkShell.main(args)
+    baos.flush


If the return type of method or function is Unit, adding parenthesis after name is recommended.

Actually the official reasoning is if it has some "side-effects" it should have some parenthesis (which it does in this case)
http://docs.scala-lang.org/style/method-invocation.html

Yeah, right. And almost all the method of which return type is Unit have some side-effects. :)

On Oct 8, 2015, at 5:05 PM, Nikolaas Steenbergen notifications@github.com wrote:

In flink-staging/flink-scala-shell/src/test/scala/org/apache/flink/api/scala/ScalaShellITSuite.scala:

val (c, args) = cluster match{

case Some(cl) =>

val arg = Array("remote",

cl.hostname,

Integer.toString(cl.getLeaderRPCPort))

(cl, arg)

case None =>

fail("Cluster creation failed!")

}

//start scala shell with initialized

// buffered reader for testing

FlinkShell.bufferedReader = Some(in)

FlinkShell.main(args)

baos.flush

Actually the official reasoning is if it has some "side-effects" it should have some parenthesis (which it does in this case)
http://docs.scala-lang.org/style/method-invocation.html

—
Reply to this email directly or view it on GitHub.

Regards,
Chiwan Park

nikste · 2015-10-08T14:46:17Z

@chiwanpark issues should be fixed now..

changed startup of scala shell user has to specify local or remote mode explicitly now.

nikste · 2015-10-08T15:06:01Z

@chiwanpark issues should be fixed NOW ;)

chiwanpark · 2015-10-09T14:39:17Z

Merging...

- Change startup code of scala shell - User has to specify local or remote mode explicitly now. This closes apache#1106.

nikste force-pushed the FLINK-2613-enhanced-help-message branch 4 times, most recently from 9ef4470 to f835a0f Compare September 14, 2015 13:29

nikste force-pushed the FLINK-2613-enhanced-help-message branch 3 times, most recently from 659f868 to e714c7a Compare September 16, 2015 12:30

fhueske reviewed Sep 16, 2015
View reviewed changes

nikste force-pushed the FLINK-2613-enhanced-help-message branch 2 times, most recently from 156716e to 135ca75 Compare September 17, 2015 12:31

fhueske reviewed Sep 17, 2015
View reviewed changes

nikste force-pushed the FLINK-2613-enhanced-help-message branch from 135ca75 to b3c7d01 Compare September 18, 2015 08:33

nikste force-pushed the FLINK-2613-enhanced-help-message branch from b3c7d01 to 40f4253 Compare September 21, 2015 08:32

nikste closed this Sep 28, 2015

nikste force-pushed the FLINK-2613-enhanced-help-message branch from 40f4253 to 2cc5d98 Compare September 28, 2015 14:51

chiwanpark mentioned this pull request Sep 30, 2015

[FLINK-2767] [scala shell] Add Scala 2.11 support to Scala shell #1197

Closed

nikste reopened this Sep 30, 2015

chiwanpark reviewed Oct 8, 2015
View reviewed changes

nikste force-pushed the FLINK-2613-enhanced-help-message branch from d4997d0 to 0bad350 Compare October 8, 2015 12:16

chiwanpark reviewed Oct 8, 2015
View reviewed changes

nikste force-pushed the FLINK-2613-enhanced-help-message branch 2 times, most recently from 05c06dc to 5fb4738 Compare October 8, 2015 14:44

[FLINK-2613] Print usage information for Scala Shell

e3e311b

changed startup of scala shell user has to specify local or remote mode explicitly now.

nikste force-pushed the FLINK-2613-enhanced-help-message branch from 5fb4738 to e3e311b Compare October 8, 2015 14:57

asfgit closed this in 7750a1f Oct 9, 2015

cfmcgrady pushed a commit to cfmcgrady/flink that referenced this pull request Oct 23, 2015

[FLINK-2613] [scala shell] Print usage information for Scala Shell

671bf87

- Change startup code of scala shell - User has to specify local or remote mode explicitly now. This closes apache#1106.

lofifnc pushed a commit to lofifnc/flink that referenced this pull request Oct 23, 2015

[FLINK-2613] [scala shell] Print usage information for Scala Shell

0fd3110

- Change startup code of scala shell - User has to specify local or remote mode explicitly now. This closes apache#1106.

rmetzger added the component=ScalaShell label Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-2613] Print usage information for Scala Shell #1106

[FLINK-2613] Print usage information for Scala Shell #1106

nikste commented Sep 8, 2015

StephanEwen commented Sep 9, 2015

fhueske commented Sep 15, 2015

fhueske Sep 16, 2015

nikste commented Sep 17, 2015

fhueske Sep 17, 2015

fhueske commented Sep 17, 2015

rmetzger commented Sep 18, 2015

nikste commented Sep 21, 2015

chiwanpark commented Sep 25, 2015

nikste commented Sep 30, 2015

sachingoel0101 commented Sep 30, 2015

chiwanpark Oct 8, 2015

chiwanpark commented Oct 8, 2015

nikste commented Oct 8, 2015

chiwanpark commented Oct 8, 2015

chiwanpark Oct 8, 2015

chiwanpark commented Oct 8, 2015

chiwanpark Oct 8, 2015

nikste Oct 8, 2015

chiwanpark Oct 8, 2015

nikste commented Oct 8, 2015

nikste commented Oct 8, 2015

chiwanpark commented Oct 9, 2015

[FLINK-2613] Print usage information for Scala Shell #1106

[FLINK-2613] Print usage information for Scala Shell #1106

Conversation

nikste commented Sep 8, 2015

StephanEwen commented Sep 9, 2015

fhueske commented Sep 15, 2015

fhueske Sep 16, 2015

Choose a reason for hiding this comment

nikste commented Sep 17, 2015

fhueske Sep 17, 2015

Choose a reason for hiding this comment

fhueske commented Sep 17, 2015

rmetzger commented Sep 18, 2015

nikste commented Sep 21, 2015

chiwanpark commented Sep 25, 2015

nikste commented Sep 30, 2015

sachingoel0101 commented Sep 30, 2015

chiwanpark Oct 8, 2015

Choose a reason for hiding this comment

chiwanpark commented Oct 8, 2015

nikste commented Oct 8, 2015

chiwanpark commented Oct 8, 2015

chiwanpark Oct 8, 2015

Choose a reason for hiding this comment

chiwanpark commented Oct 8, 2015

chiwanpark Oct 8, 2015

Choose a reason for hiding this comment

nikste Oct 8, 2015

Choose a reason for hiding this comment

chiwanpark Oct 8, 2015

Choose a reason for hiding this comment

nikste commented Oct 8, 2015

nikste commented Oct 8, 2015

chiwanpark commented Oct 9, 2015