This repository was archived by the owner on Nov 22, 2024. It is now read-only.
Operator on Fabric8 actions#994
Merged
Merged
Conversation
| val thisService = | ||
| services.inNamespace(service.getMetadata.getNamespace).withName(service.getMetadata.getName) | ||
| val ser = thisService.get() | ||
| Option(ser) match { |
Contributor
There was a problem hiding this comment.
Maybe do Option(thisService.get()) so no one can make the mistake to use ser (super minor)
| val metadataName = newEventName(podName, app.spec.appId) | ||
|
|
||
| // the object reference fieldPath is irrelevant for application events. | ||
| fieldPath match { |
Contributor
There was a problem hiding this comment.
minor, use foreach?
| ownerReferences: List[OwnerReference]): Seq[Action] = | ||
| app.spec.streamlets | ||
| .map(streamlet => streamlet.descriptor.runtime.name) | ||
| .map(streamlet => streamlet.descriptor.runtime) |
Contributor
There was a problem hiding this comment.
Why was this change needed? (just does not seem fabric8 related)
Contributor
Author
There was a problem hiding this comment.
it's related to the usage of the Fabric8 CR
|
|
||
| private val log = LoggerFactory.getLogger(TopicActions.getClass) | ||
|
|
||
| // TOOD: again Blueprint and CR overlapping |
Contributor
There was a problem hiding this comment.
Suggested change
| // TOOD: again Blueprint and CR overlapping | |
| // TODO: again Blueprint and CR overlapping |
Contributor
There was a problem hiding this comment.
I think we know what to do about this, so maybe remove the comment.
|
|
||
| val runtime = Runtime | ||
|
|
||
| def createOrReplaceResource(res: Deployment)(implicit ct: ClassTag[Deployment]): Action = { |
Contributor
Author
There was a problem hiding this comment.
used in the Runner
| Action.createOrReplace(res) | ||
| } | ||
|
|
||
| def deleteResource(name: String, namespace: String)(implicit ct: ClassTag[Deployment]): Action = |
| .inNamespace(deployment.getMetadata.getNamespace) | ||
| .withName(deployment.getMetadata.getName) | ||
| .patch(deployment) | ||
| Action.log.warn("Akka deployment patched!") |
Contributor
There was a problem hiding this comment.
Why warn? maybe debug leftover.
| .withName(deployment.getMetadata.getName) | ||
| .patch(deployment) | ||
| Action.log.warn("Akka deployment patched!") | ||
| Action.noop |
Contributor
There was a problem hiding this comment.
Why return noop? you could return this?
| app: App.Cr, | ||
| configSecret: Secret, | ||
| updateLabels: Map[String, String] = Map()): Deployment = { | ||
| // The runtimeConfig is already applied in the runner config secret, so it can be safely ignored. |
Contributor
There was a problem hiding this comment.
Which runtimeConfig? maybe outdated comment, remove?
| // Pass this argument to the entry point script. The top level entry point will be a | ||
| // cloudflow-entrypoint.sh which will route to the appropriate entry point based on the | ||
| // arguments passed to it | ||
| val args = List("akka") |
Contributor
There was a problem hiding this comment.
I think this can be removed, the entrypoint is now already akka specific, so please remove comment and these args.
| .withResources(resourceRequirements) | ||
| .withImage(deployment.image) | ||
| .withEnv(environmentVariables: _*) | ||
| .withArgs(args: _*) |
| Map("cpu" -> resourceConstraints.cpuRequests, "memory" -> resourceConstraints.memoryRequests) | ||
| } | ||
|
|
||
| lazy val resourceRequirements = |
Contributor
There was a problem hiding this comment.
Suggested change
| lazy val resourceRequirements = | |
| lazy val resourceRequirementsFromDefaults = |
| containerConfig <- pod.containers.get(PodsConfig.CloudflowContainerName) | ||
| resources <- containerConfig.resources | ||
| } yield { | ||
| resources |
Contributor
There was a problem hiding this comment.
you miss the defaults here.
| var resReqManagerBuilder = new ResourceRequirementsBuilder() | ||
|
|
||
| val defaultRequests = | ||
| ((resourceDefaults.cpuRequest match { |
| case Some(req) => Map("cpu" -> req) | ||
| case _ => Map.empty[String, Quantity] | ||
| }) ++ | ||
| (resourceDefaults.memoryRequest match { |
| case Some(secret) => | ||
| createOrReplaceResource(resource(deployment, newApp, secret)) | ||
| case None => | ||
| // TODO: this is an un-recoverable error -> errorAction instead? @Ray |
Contributor
There was a problem hiding this comment.
Let's not change this now.
| .build() | ||
| } | ||
|
|
||
| val resources = { |
| updatedExecutor | ||
| } | ||
|
|
||
| // TODO: changed the original logic that seem to be incorrect |
Contributor
There was a problem hiding this comment.
We discussed, can remove comment.
| Option(typedSelector.fromServer().get()) match { | ||
| case Some(curr) => | ||
| // NOTE: the typed API for patching doesn't work ... | ||
| Action.log.warn(s"Patching Spark Cr ${cr.name} in ${cr.namespace}") |
| case _ => Nil // app could not be found, do nothing. | ||
| } | ||
|
|
||
| // TODO: duplicated fixme |
|
|
||
| import scala.jdk.CollectionConverters._ | ||
|
|
||
| class CloudflowApplicationSpec |
| libraryDependencies ++= Seq( | ||
| Compile.typesafeConfig, | ||
| Compile.sprayJson, | ||
| // TODO: check if Avro and ScalaPB can stay in a separate module |
RayRoestenburg
approved these changes
Mar 22, 2021
RayRoestenburg
left a comment
RayRoestenburg
left a comment
Contributor
There was a problem hiding this comment.
LGTM in general, some minor comments, and the fix for pod config resource requirements.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opening a Draft PR to monitor the progress and to share.
Current status:
TODO: