You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gearpump.apache.org by huafengw <gi...@git.apache.org> on 2017/01/17 11:22:54 UTC
[GitHub] incubator-gearpump pull request #133: GEARPUMP-252 return meaningful result ...
GitHub user huafengw opened a pull request:
https://github.com/apache/incubator-gearpump/pull/133
GEARPUMP-252 return meaningful result than app id when submitting app\u2026
\u2026lication in ClientContext
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
- [ ] Make sure the commit message is formatted like:
`[GEARPUMP-<Jira issue #>] Meaningful description of pull request`
- [ ] Make sure tests pass via `sbt clean test`.
- [ ] Make sure old documentation affected by the pull request has been updated and new documentation added for new functionality.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/huafengw/incubator-gearpump refactorClient
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-gearpump/pull/133.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #133
----
commit bd2c0702d7a2e5d5e3e26675301c46900ba14ed8
Author: huafengw <fv...@gmail.com>
Date: 2017-01-17T11:21:36Z
GEARPUMP-252 return meaningful result than app id when submitting application in ClientContext
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-gearpump/pull/133
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by huafengw <gi...@git.apache.org>.
Github user huafengw commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96557250
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/ClientContext.scala ---
@@ -53,20 +55,21 @@ class ClientContext(config: Config, sys: ActorSystem, _master: ActorRef) {
}
private val LOG: Logger = LogUtil.getLogger(getClass)
- private implicit val timeout = Timeout(5, TimeUnit.SECONDS)
-
implicit val system = Option(sys).getOrElse(ActorSystem(s"client${Util.randInt()}", config))
LOG.info(s"Starting system ${system.name}")
- val shouldCleanupSystem = Option(sys).isEmpty
-
+ private val shouldCleanupSystem = Option(sys).isEmpty
private val jarStoreClient = new JarStoreClient(config, system)
+ private val masterClientTimeout = {
+ val timeout = Try(config.getInt(Constants.GEARPUMP_MASTERCLIENT_TIMEOUT)).getOrElse(90)
--- End diff --
It's inherited from the old impl
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump issue #133: [GEARPUMP-252] return meaningful result than ...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on the issue:
https://github.com/apache/incubator-gearpump/pull/133
+1, merging
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96553343
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/ClientContext.scala ---
@@ -35,14 +36,15 @@ import org.slf4j.Logger
import scala.collection.JavaConverters._
import scala.concurrent.duration.Duration
import scala.concurrent.{Await, Future}
-import scala.util.Try
+import scala.util.{Failure, Success, Try}
/**
* ClientContext is a user facing util to submit/manage an application.
*
* TODO: add interface to query master here
*/
class ClientContext(config: Config, sys: ActorSystem, _master: ActorRef) {
+ import scala.concurrent.ExecutionContext.Implicits.global
--- End diff --
better put this in the headers
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by huafengw <gi...@git.apache.org>.
Github user huafengw commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96557102
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/ClientContext.scala ---
@@ -53,20 +55,21 @@ class ClientContext(config: Config, sys: ActorSystem, _master: ActorRef) {
}
private val LOG: Logger = LogUtil.getLogger(getClass)
- private implicit val timeout = Timeout(5, TimeUnit.SECONDS)
-
implicit val system = Option(sys).getOrElse(ActorSystem(s"client${Util.randInt()}", config))
LOG.info(s"Starting system ${system.name}")
- val shouldCleanupSystem = Option(sys).isEmpty
-
+ private val shouldCleanupSystem = Option(sys).isEmpty
--- End diff --
The `system` won't be empty but `sys` can still be empty. `sys` not being empty means user has provided a ActorSystem so clientcontext should clean up user's actor system when being closed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96554802
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/ClientContext.scala ---
@@ -75,59 +78,67 @@ class ClientContext(config: Config, sys: ActorSystem, _master: ActorRef) {
* defined. Otherwise, it assumes the jar is on the target runtime classpath, thus will
* not send the jar across the wire.
*/
- def submit(app: Application): Int = {
+ def submit(app: Application): RunningApplication = {
submit(app, System.getProperty(GEARPUMP_APP_JAR))
}
- def submit(app: Application, jar: String): Int = {
- submit(app, jar, getExecutorNum())
+ def submit(app: Application, jar: String): RunningApplication = {
+ submit(app, jar, getExecutorNum)
}
- def submit(app: Application, jar: String, executorNum: Int): Int = {
- val client = getMasterClient
+ def submit(app: Application, jar: String, executorNum: Int): RunningApplication = {
val appName = checkAndAddNamePrefix(app.name, System.getProperty(GEARPUMP_APP_NAME_PREFIX))
val submissionConfig = getSubmissionConfig(config)
.withValue(APPLICATION_EXECUTOR_NUMBER, ConfigValueFactory.fromAnyRef(executorNum))
val appDescription =
AppDescription(appName, app.appMaster.getName, app.userConfig, submissionConfig)
val appJar = Option(jar).map(loadFile)
- client.submitApplication(appDescription, appJar)
+ submitApplication(SubmitApplication(appDescription, appJar))
}
- private def getExecutorNum(): Int = {
+ private def getExecutorNum: Int = {
Try(System.getProperty(APPLICATION_EXECUTOR_NUMBER).toInt).getOrElse(1)
}
private def getSubmissionConfig(config: Config): Config = {
ClusterConfig.filterOutDefaultConfig(config)
}
+ def listApps: AppMastersData = {
+ ActorUtil.askActor[AppMastersData](master, AppMastersDataRequest, masterClientTimeout)
+ }
+
+ @Deprecated
--- End diff --
If these methods are deprecated, which should be used instead ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96554021
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/ClientContext.scala ---
@@ -53,20 +55,21 @@ class ClientContext(config: Config, sys: ActorSystem, _master: ActorRef) {
}
private val LOG: Logger = LogUtil.getLogger(getClass)
- private implicit val timeout = Timeout(5, TimeUnit.SECONDS)
-
implicit val system = Option(sys).getOrElse(ActorSystem(s"client${Util.randInt()}", config))
LOG.info(s"Starting system ${system.name}")
- val shouldCleanupSystem = Option(sys).isEmpty
-
+ private val shouldCleanupSystem = Option(sys).isEmpty
private val jarStoreClient = new JarStoreClient(config, system)
+ private val masterClientTimeout = {
+ val timeout = Try(config.getInt(Constants.GEARPUMP_MASTERCLIENT_TIMEOUT)).getOrElse(90)
--- End diff --
why is the default value `90` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96553846
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/ClientContext.scala ---
@@ -53,20 +55,21 @@ class ClientContext(config: Config, sys: ActorSystem, _master: ActorRef) {
}
private val LOG: Logger = LogUtil.getLogger(getClass)
- private implicit val timeout = Timeout(5, TimeUnit.SECONDS)
-
implicit val system = Option(sys).getOrElse(ActorSystem(s"client${Util.randInt()}", config))
LOG.info(s"Starting system ${system.name}")
- val shouldCleanupSystem = Option(sys).isEmpty
-
+ private val shouldCleanupSystem = Option(sys).isEmpty
--- End diff --
I'm thinking how `sys` can ever be empty here since we just created one above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96554845
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/RunningApplication.scala ---
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gearpump.cluster.client
+
+import akka.pattern.ask
+import akka.actor.ActorRef
+import akka.util.Timeout
+import org.apache.gearpump.cluster.ClientToMaster.{ResolveAppId, ShutdownApplication}
+import org.apache.gearpump.cluster.MasterToClient.{ResolveAppIdResult, ShutdownApplicationResult}
+import org.apache.gearpump.util.ActorUtil
+
+import scala.concurrent.Future
+import scala.util.{Failure, Success}
+
+class RunningApplication(val appId: Int, master: ActorRef, timeout: Timeout) {
--- End diff --
Any UT for this class ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96554180
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/ClientContext.scala ---
@@ -161,13 +172,24 @@ class ClientContext(config: Config, sys: ActorSystem, _master: ActorRef) {
fullName
}
- private def getMasterClient: MasterClient = {
- val timeout = Try(config.getInt(Constants.GEARPUMP_MASTERCLIENT_TIMEOUT)).getOrElse(90)
- new MasterClient(master, akka.util.Timeout(timeout, TimeUnit.SECONDS))
+ private def submitApplication(submitApplication: SubmitApplication): RunningApplication = {
+ val result = ActorUtil.askActor[SubmitApplicationResult](master,
+ submitApplication, masterClientTimeout)
+ val application = result.appId match {
+ case Success(appId) =>
+ // scalastyle:off println
+ Console.println(s"Submit application succeed. The application id is $appId")
+ // scalastyle:on println
+ new RunningApplication(appId, master, masterClientTimeout)
+ case Failure(ex) => throw ex
+ }
+ application
}
}
object ClientContext {
+ // This magic number is derived from Akka's configuration
+ final val INFINITE_TIMEOUT = Timeout(2147482, TimeUnit.SECONDS)
--- End diff --
which configuration ? Any links ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96554483
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/RunningApplication.scala ---
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gearpump.cluster.client
+
+import akka.pattern.ask
+import akka.actor.ActorRef
+import akka.util.Timeout
+import org.apache.gearpump.cluster.ClientToMaster.{ResolveAppId, ShutdownApplication}
+import org.apache.gearpump.cluster.MasterToClient.{ResolveAppIdResult, ShutdownApplicationResult}
+import org.apache.gearpump.util.ActorUtil
+
+import scala.concurrent.Future
+import scala.util.{Failure, Success}
+
+class RunningApplication(val appId: Int, master: ActorRef, timeout: Timeout) {
+ import scala.concurrent.ExecutionContext.Implicits.global
+ lazy val appMaster: ActorRef = resolveAppMaster(appId)
+
+ def shutDown(): Unit = {
+ val result = ActorUtil.askActor[ShutdownApplicationResult](master,
+ ShutdownApplication(appId), timeout)
+ result.appId match {
+ case Success(_) =>
+ case Failure(ex) => throw ex
+ }
+ }
+
+ def askMaster[T](msg: Any): Future[T] = {
+ appMaster.ask(msg)(timeout).asInstanceOf[Future[T]]
+ }
+
+// Todo
--- End diff --
I'm afraid the intention for these lines could be forgotten after a while so maybe better just remove them for now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump issue #133: [GEARPUMP-252] return meaningful result than ...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/incubator-gearpump/pull/133
## [Current coverage](https://codecov.io/gh/apache/incubator-gearpump/pull/133?src=pr) is 70.34% (diff: 63.88%)
> Merging [#133](https://codecov.io/gh/apache/incubator-gearpump/pull/133?src=pr) into [master](https://codecov.io/gh/apache/incubator-gearpump/branch/master?src=pr) will decrease coverage by **0.15%**
```diff
@@ master #133 diff @@
==========================================
Files 191 191
Lines 6020 6022 +2
Methods 5501 5503 +2
Messages 0 0
Branches 517 517
==========================================
- Hits 4244 4236 -8
- Misses 1776 1786 +10
Partials 0 0
```
> Powered by [Codecov](https://codecov.io?src=pr). Last update [636cd6f...bd2c070](https://codecov.io/gh/apache/incubator-gearpump/compare/636cd6f8ef566260932848d2cc1b6c77fd8c90b3...bd2c0702d7a2e5d5e3e26675301c46900ba14ed8?src=pr)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump issue #133: [GEARPUMP-252] return meaningful result than ...
Posted by huafengw <gi...@git.apache.org>.
Github user huafengw commented on the issue:
https://github.com/apache/incubator-gearpump/pull/133
@manuzhang
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-gearpump pull request #133: [GEARPUMP-252] return meaningful resul...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/incubator-gearpump/pull/133#discussion_r96554587
--- Diff: core/src/main/scala/org/apache/gearpump/cluster/client/RunningApplication.scala ---
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gearpump.cluster.client
+
+import akka.pattern.ask
+import akka.actor.ActorRef
+import akka.util.Timeout
+import org.apache.gearpump.cluster.ClientToMaster.{ResolveAppId, ShutdownApplication}
+import org.apache.gearpump.cluster.MasterToClient.{ResolveAppIdResult, ShutdownApplicationResult}
+import org.apache.gearpump.util.ActorUtil
+
+import scala.concurrent.Future
+import scala.util.{Failure, Success}
+
+class RunningApplication(val appId: Int, master: ActorRef, timeout: Timeout) {
+ import scala.concurrent.ExecutionContext.Implicits.global
--- End diff --
better put this in the header
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---