text:
[SPARK-8167] Make tasks that fail from YARN preemption not fail job

The architecture is that, in YARN mode, if the driver detects that an executor has disconnected, it asks the ApplicationMaster why the executor died. If the ApplicationMaster is aware that the executor died because of preemption, all tasks associated with that executor are not marked as failed. The executor
is still removed from the driver's list of available executors, however.
There's a few open questions:

Should standalone mode have a similar "get executor loss reason" as well? I localized this change as much as possible to affect only YARN, but there could be a valid case to differentiate executor losses in standalone mode as well.
I make a pretty strong assumption in YarnAllocator that getExecutorLossReason(executorId) will only be called once per executor id; I do this so that I can remove the metadata from the in-memory map to avoid object accumulation. It's not clear if I'm being overly zealous to save space, however.

cc @vanzin specifically for review because it collided with some earlier YARN scheduling work.
cc @JoshRosen because it's similar to output commit coordination we did in the past
cc @andrewor14 for our discussion on how to get executor exit codes and loss reasons

Tests I ran locally were: set spark.task.maxFailures to 1, create a spark shell and start a job that just sleeps all of my executor threads for a long time. While the job is running, kick off a SparkPi against a different queue, such that my spark shell executors are preempted. The preemption doesn't make the spark shell crash, which would not have been the case from before this PR.
Additional unit test coverage suggestions are appreciated though.

Merged build triggered.

Merged build started.

Test build #40083 has started for   PR 8007 at commit aa69b6f.

Test build #40083 has finished for   PR 8007 at commit aa69b6f.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorForTaskExited(taskId: Long, execId: String, exitReason: String, exitCode: Int) extends TaskFailedReason
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason) extends CoarseGrainedClusterMessage
  case class GetExecutorLossReason(executorId: String)

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

Test build #40116 has started for   PR 8007 at commit e3e827f.

Test build #40116 has finished for   PR 8007 at commit e3e827f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorForTaskExited(
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String)

Merged build finished. Test FAILed.

Jenkins, retest this please.

Merged build triggered.

Merged build started.

Test build #40180 has started for   PR 8007 at commit e3e827f.

Test build #40180 has finished for   PR 8007 at commit e3e827f.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorForTaskExited(
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String)

Merged build finished. Test FAILed.

Jenkins, retest this please

Seems like flaky tests? The failing tests don't seem to be touching the scheduling components I changed.

Merged build triggered.

Merged build started.

Test build #40192 has started for   PR 8007 at commit e3e827f.

Test build #40192 has finished for   PR 8007 at commit e3e827f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorForTaskExited(
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String)

Merged build finished. Test FAILed.

Jenkins, retest this please

Merged build triggered.

Merged build started.

Test build #40212 has started for   PR 8007 at commit e3e827f.

Test build #40212 has finished for   PR 8007 at commit e3e827f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorForTaskExited(
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String)

Merged build finished. Test PASSed.

Build triggered.

Build started.

Test build #40792 has started for   PR 8007 at commit 8ee0c45.

Test build #40792 has finished for   PR 8007 at commit 8ee0c45.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
case class ExecutorNormalExit(
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String)

Build finished. Test FAILed.

Hey @mccheah, good stuff. Turns out I have a similar pull request lingering: #8093
In that pull request, I discuss the same race condition you are tackling here (although I don't think you talk about it explicitly) whereby an onDisconnected event may be received by the driver before a RemoveExecutor event. The manifestation of that race, that I am most concerned about is the correct reason showing the UI if Yarn killed the container.
I had been playing with a few ideas on how to do that. The first one I hacked was using a new message for SparkListenerBus. If the onDisconnected() event was received before RemoveExecutor, when the RemoveExecutor with the real error would arrive, a new message would be sent down the listener bus with the update reason for the killing of that executor. The code for that up at https://github.com/markgrover/spark/compare/nm2...race just fyi.
That's just a preliminary hack I did to see if that works, and it seems like it does work pretty well.
However, after looking at your PR, I think you solve the same problem, albeit in a more general way. And, I think that may solve my problem too, perhaps with some extension. That's because with your suggested change, regardless of whether onDisconnected or RemoveExecutor is received first, they both have a legitimate reason, in legitimate cases.
Anyways, I just wanted to let you know where my head is at, and I may pull down some of your bits and pieces from this PR, into mine and it see if we need to make any updates to those pieces to solve my problem.

Yeah I noticed that removeExecutor was fired from the application master as well. That's okay - in this code I specifically also encode the executor loss reason in the removeExecutor message whether or not Application master sends it or the DriverEndpoint sends it (via onDisconnected). But I did have to specifically override DriverEndpoint to make sure it sends the same message that the ApplicationMaster would have.

Build triggered.

Build started.

Test build #40832 has started for   PR 8007 at commit 601164f.

Merged build triggered.

Merged build started.

Merged build finished. Test FAILed.

Test build #40832 timed out     for PR 8007 at commit 601164f     after a configured wait of 175m.

Build finished. Test FAILed.

Also, I am borrowing some of your ideas in my pull request and indadvertedly validating some of your work:-) So far, so good but I am getting quite a few exceptions when sending the GetExecutorLossReason message - a little too many for my comfort. Out of 74 executors that got removed in my app in yarn client mode, 47 of them threw such an exception. Here's the full stack trace:
15/08/14 09:21:44 WARN AkkaRpcEndpointRef: Error sending message [message = GetExecutorLossReason(2)] in 1 attempts
java.util.concurrent.ExecutionException: Boxed Error
        at scala.concurrent.impl.Promise$.resolver(Promise.scala:55)
        at scala.concurrent.impl.Promise$.scala$concurrent$impl$Promise$$resolveTry(Promise.scala:47)        at scala.concurrent.impl.Promise$KeptPromise.<init>(Promise.scala:324)
        at scala.concurrent.Promise$.failed(Promise.scala:134)        at scala.concurrent.Future$.failed(Future.scala:467)
        at org.apache.spark.rpc.akka.AkkaRpcEndpointRef$$anonfun$ask$1.apply(AkkaRpcEnv.scala:317)        at org.apache.spark.rpc.akka.AkkaRpcEndpointRef$$anonfun$ask$1.apply(AkkaRpcEnv.scala:307)
        at scala.concurrent.Future$$anonfun$flatMap$1.apply(Future.scala:251)
        at scala.concurrent.Future$$anonfun$flatMap$1.apply(Future.scala:249)        at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:32)
        at org.spark-project.guava.util.concurrent.MoreExecutors$SameThreadExecutorService.execute(MoreExecutors.java:293
)
        at scala.concurrent.impl.ExecutionContextImpl$$anon$1.execute(ExecutionContextImpl.scala:133)
        at scala.concurrent.impl.CallbackRunnable.executeWithValue(Promise.scala:40)
        at scala.concurrent.impl.Promise$DefaultPromise.tryComplete(Promise.scala:248)
        at akka.pattern.PromiseActorRef.$bang(AskSupport.scala:266)
        at akka.remote.DefaultMessageDispatcher.dispatch(Endpoint.scala:89)
        at akka.remote.EndpointReader$$anonfun$receive$2.applyOrElse(Endpoint.scala:935)
        at akka.actor.Actor$class.aroundReceive(Actor.scala:467)        at akka.remote.EndpointActor.aroundReceive(Endpoint.scala:411)
        at akka.actor.ActorCell.receiveMessage(ActorCell.scala:516)
        at akka.actor.ActorCell.invoke(ActorCell.scala:487)        at akka.dispatch.Mailbox.processMailbox(Mailbox.scala:238)
        at akka.dispatch.Mailbox.run(Mailbox.scala:220)        at akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(AbstractDispatcher.scala:397)
        at scala.concurrent.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)        at scala.concurrent.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
        at scala.concurrent.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)        at scala.concurrent.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)Caused by: java.lang.AssertionError: assertion failed
        at scala.Predef$.assert(Predef.scala:165)
        at org.apache.spark.deploy.yarn.YarnAllocator.getExecutorLossReason(YarnAllocator.scala:215)
        at org.apache.spark.deploy.yarn.ApplicationMaster$AMEndpoint$$anonfun$receiveAndReply$1.applyOrElse(ApplicationMaster.scala:591)
        at org.apache.spark.rpc.akka.AkkaRpcEnv.org$apache$spark$rpc$akka$AkkaRpcEnv$$processMessage(AkkaRpcEnv.scala:177)
        at org.apache.spark.rpc.akka.AkkaRpcEnv$$anonfun$actorRef$lzycompute$1$1$$anon$1$$anonfun$receiveWithLogging$1$$anonfun$applyOrElse$4.apply$mcV$sp(AkkaRpcEnv.scala:126)
        at org.apache.spark.rpc.akka.AkkaRpcEnv.org$apache$spark$rpc$akka$AkkaRpcEnv$$safelyCall(AkkaRpcEnv.scala:197)
        at org.apache.spark.rpc.akka.AkkaRpcEnv$$anonfun$actorRef$lzycompute$1$1$$anon$1$$anonfun$receiveWithLogging$1.applyOrElse(AkkaRpcEnv.scala:125)
        at scala.runtime.AbstractPartialFunction$mcVL$sp.apply$mcVL$sp(AbstractPartialFunction.scala:33)
        at scala.runtime.AbstractPartialFunction$mcVL$sp.apply(AbstractPartialFunction.scala:33)
        at scala.runtime.AbstractPartialFunction$mcVL$sp.apply(AbstractPartialFunction.scala:25)
        at org.apache.spark.util.ActorLogReceive$$anon$1.apply(ActorLogReceive.scala:59)
        at org.apache.spark.util.ActorLogReceive$$anon$1.apply(ActorLogReceive.scala:42)
        at scala.PartialFunction$class.applyOrElse(PartialFunction.scala:118)
        at org.apache.spark.util.ActorLogReceive$$anon$1.applyOrElse(ActorLogReceive.scala:42)
        at akka.actor.Actor$class.aroundReceive(Actor.scala:467)
        at org.apache.spark.rpc.akka.AkkaRpcEnv$$anonfun$actorRef$lzycompute$1$1$$anon$1.aroundReceive(AkkaRpcEnv.scala:92)
        ... 9 more

Anyways, I'll poke a little more but wanted to post here to keep you in the loop.

Got it - was hoping GetExecutorLossReason wouldn't be called more than once. That is what's allowing me to clean up the map there.
In general I may have been overly paranoid in this PR to attempt to clean up data structures by removing items from maps. I'm not sure what the ramifications are of having the maps keep filling up however - presumably that would cause problems for really long-running Spark Contexts. If anyone has any better suggestions on cleanup logic, that would be appreciated.

Haven't tested this attempted fix yet - @markgrover feel free to try it. Pushing to github so I can pick it up on my linux boxes (my mac is having trouble with preemption in general for some reason)

Merged build triggered.

Merged build started.

Test build #41082 has started for   PR 8007 at commit 70d6a15.

Actually @markgrover can you describe in more detail how you were trying to use GetExecutorLossReason? I think just using the code in this PR, my old implementation should have still never called GetExecutorLossReason twice, assuming that processing the RemoveExecutor event can never run in parallel with processing the onDisconnected event. This new implementation is (IMO) cleaner however in favoring the use of the Akka event loop a bit more instead of the synchronized primitive.

Test build #41082 has finished for   PR 8007 at commit 70d6a15.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorNormalExit(
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class AcknowledgeExecutorRemoved(executorId: String) extends CoarseGrainedClusterMessage
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41082/
Test FAILed.

Yeah, for sure, let me upload the logs on gist and explain what I am talking about. Will do that soon.

Cool - and once again, trying the new commit would be appreciated.
Also @markgrover  how do we want to resolve all of the duplicate work being done here and in #8093? Should we try to merge this commit first and have your commit be rebased on top of this? Or should it be the other way around?

Should we try to merge this commit first and have your commit be rebased on top of this?

I think that's the best approach; Mark can keep some (or all) of the code here in his branch for now so that he can test things at least. (And sorry I've been procrastinating here - I still plan to review this.)

Jenkins, retest this please

Merged build triggered.

Merged build started.

Test build #41152 has started for   PR 8007 at commit 70d6a15.

Test build #41152 has finished for   PR 8007 at commit 70d6a15.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorNormalExit(
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class AcknowledgeExecutorRemoved(executorId: String) extends CoarseGrainedClusterMessage
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41152/
Test FAILed.

This is definitely going in the right direction; there's a couple of things that I think need some further investigation, though.

Actually @markgrover can you describe in more detail how you were trying to use GetExecutorLossReason?

So, I uploaded my YARN and driver logs.
See here as to how the Executor Loss Reason is being asked for twice. I asserted that it was only requested once by looking at the driver log. You can do so too by searching for Requesting loss reason for executorId: 2 in the full driver log.
The relevant event receiving code snippet is here and the relevant event sending code snippet is here.
This is all before merging your latest change since I was trying this out last Friday. I wasn't able to figure out then why 2 events were being received.
Anyways, I realize this is a lot of info and perhaps, hard to go through. I will poke at it more, perhaps, merge your latest commit and see if that helps. I am envisioning some pain given that your and my pull requests are sharing some code. If it continues for much longer, may be we should work off of the same branch. I personally am indifferent to whether I rebase on yours, or vice-versa.
I always also hoping to find a spark IRC channel where we could collaborate real time but couldn't. I would definitely be open to hacking on this in a more collaborative way if you think it'd help (I think it would).

So my next steps: I'll go with @vanzin 's suggestion for using RpcEndpointRef.ask instead and I think that doing that the right way invalidates a lot of the multiple-get-executor-loss-reason race condition nastiness @markgrover is seeing. As for making sure tasks aren't temporarily being allocated on dead executors, I'll possibly tackle that separately to keep commits smaller in scope.
The latter problem seems harder and would involve a concerning amount of code churn, and I'm not sure if I want to expand this commit any more than I have to... but making more jobs fail because tasks are allocated to bad executors and hence contribute to spark.task.maxFailures (in the case that the executor failure is fatal, so to speak) is a non-trivial issue.

So one case this doesn't cover is if the ApplicationMaster is preempted. I'm not sure what the exact behavior is there but if the AM is preempted then attempts to get the executor loss reason will certainly time out because the new AM that registers won't know about the previous executors. So if the AM dies along with the executors then getting the loss reason for those killed executors will fail. I suppose we can only do a best-effort here however - special casing for when the ApplicationMaster is killed will be difficult to get right.

Your assumption probably holds for the preemption case, since it's YARN killing the container. But I can imagine that if the container exits by itself, it might be possible for the disconnect to reach the driver endpoint and the GetExecutorLossReason message to reach the AM before the NM has had a chance to process the container exit and communicate that to the RM.

There is still something I don't quite understand here so posting for my self-educational purposes.

In my case, it was still YARN killing the container, albeit, not for pre-emption but for other reasons and I would have hoped/assumed that the AM state would have updated by RM by the time we request the loss reason. It seems like that may not always be the case, which I wasn't expecting.

I was getting two GetExecutorLossReason messages in the AM, but the only place that can send it is the scheduler backend when it gets the disconnected event. That in my opinion is orthogonal to us figuring out the answer to (1) above So, for shits and giggles, I am going to try to keep the entries around in pendingDisconnectedExecutors and see if that prevents the two calls and will post back.

In my case, it was still YARN killing the container

I should have been more specific; in the preemption case, it's the RM deciding whether to kill a container. So before the container is killed, the RM already knows it's going away, and can potentially report that. I don't know whether that's actually the case, which is why I wouldn't rely on that being true.
In the overhead case, it's the NM killing the container; before the AM is notified of that, the NM has to notify the RM. So there's plenty of room for races in that case.

Ah, makes sense, thanks.

New commit is largely untested however - but just wanted to post what I had before the weekend. Some comments from @vanzin still are not addressed.

Merged build triggered.

Merged build started.

Test build #41399 has started for   PR 8007 at commit 22fbe2a.

Test build #41399 has finished for   PR 8007 at commit 22fbe2a.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorNormalExit(
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41399/
Test FAILed.

Merged build triggered.

Merged build started.

Test build #41478 has started for   PR 8007 at commit ace86d2.

Merged build triggered.

Merged build started.

Test build #41479 has started for   PR 8007 at commit 3c4d017.

Test build #41479 has finished for   PR 8007 at commit 3c4d017.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorNormalExit(
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExitedNormally(val exitCode: Int, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41479/
Test FAILed.

Merged build triggered.

Merged build started.

Test build #41484 has started for   PR 8007 at commit 33dc729.

Test build #41478 timed out     for PR 8007 at commit ace86d2     after a configured wait of 175m.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41478/
Test FAILed.

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

Ok, I think I've addressed all the comments sans the "tasks will be submitted to dead executors temporarily" one. Still largely untested. Will be testing now, but pushing for visibility and discussion.
Thanks for all the traffic on this so far by the way. Our iteration has allowed us to refine this code quite a bit already.

Test build #41499 has started for   PR 8007 at commit 9f6b363.

Test build #41499 has finished for   PR 8007 at commit 9f6b363.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait TaskEndReason
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false) extends TaskFailedReason
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(val exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41499/
Test FAILed.

Merged build triggered.

Merged build started.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41500/
Test FAILed.

Merged build finished. Test FAILed.

Test build #41484 timed out     for PR 8007 at commit 33dc729     after a configured wait of 175m.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41484/
Test FAILed.

Jenkins, test this please

Merged build triggered.

Merged build started.

Test build #41505 has started for   PR 8007 at commit 6a53f63.

Test build #41505 has finished for   PR 8007 at commit 6a53f63.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait TaskEndReason
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(val exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41505/
Test FAILed.

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

Test build #41511 has started for   PR 8007 at commit c7f3f32.

Test build #41511 has finished for   PR 8007 at commit c7f3f32.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait TaskEndReason
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(val exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41511/
Test FAILed.

Merged build triggered.

Merged build started.

Test build #41514 has started for   PR 8007 at commit b104a68.

Hm, initial testing seems ok. I did run into a case where an asking was lost. Not immediately clear as to why, but it turned out that the executor had been removed anyways and the subsequent attempt to remove the now-non-existant executor was harmless.
You can hit a race condition where the AM reporter thread processes the disconnected executor before the onDisconnected-get-executor-loss-reason request makes it to the AM, but that's probably okay since the AM would have also sent a removeExecutor request to the driver with the right loss reason anyways...
It would be nice to have an automated way to test this, but I don't currently see any test suite for YarnSchedulerBackend and it looks fairly difficult to conjure one.

Merged build triggered.

Merged build started.

Test build #41516 has started for   PR 8007 at commit b472c58.

Test build #41514 has finished for   PR 8007 at commit b104a68.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait TaskEndReason
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(val exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41514/
Test PASSed.

Merged build finished. Test PASSed.

Test build #41516 has finished for   PR 8007 at commit b472c58.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait TaskEndReason
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(val exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build triggered.

Merged build started.

Test build #41573 has started for   PR 8007 at commit 8057da1.

Test build #41573 has finished for   PR 8007 at commit 8057da1.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait TaskEndReason
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(val exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41573/
Test FAILed.

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

Test build #41575 has started for   PR 8007 at commit 7188791.

Test build #41575 has finished for   PR 8007 at commit 7188791.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait TaskEndReason
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(val exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41575/
Test FAILed.

Merged build triggered.

Merged build started.

Test build #41576 has started for   PR 8007 at commit 5ea0b04.

Test build #41576 has finished for   PR 8007 at commit 5ea0b04.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait TaskEndReason
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(val exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41576/
Test PASSed.

@mccheah sorry for the delay. I'll have another look at this today or tomorrow.

Looking better; there are still a few things that look suspicious to me in ApplicationMaster, though.

Accidentally missed one of @andrewor14 's comments but everything else is addressed I think.
One point to note: I moved the loss-reason-request-reply logic into YarnAllocator. In order to find if a get-loss-reason-request is valid such that the request would only be enqueued if the loss reason were found on a subsequent allocateResources() call, I needed to check executorIdToContainer in YarnAllocator. But that would require synchronizing on YarnAllocator... and in my previous patch, I was also synchronizing on pendingLossReasonRequests in ApplicationMaster, so there could have potentially been nested synchronized blocks on different objects which is a little scary.
I just moved pendingExecutorLossReasonRequests into YarnAllocator and put the context replying in processCompletedContainers() instead, so we only need to synchronize strictly on YarnAllocator's methods.

Merged build triggered.

Merged build started.

Ah sorry @mccheah I was actually not done yet. I will resume now.

Merged build triggered.

Merged build started.

Test build #41956 has started for   PR 8007 at commit e967fa4.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41955/
Test FAILed.

Test build #41956 has finished for   PR 8007 at commit e967fa4.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41956/
Test FAILed.

@mccheah Overall this looks pretty good. I think throughout the patch there are still quite a few places where the change is not intended / no longer necessary. Other than those the biggest open question might be the potential race condition where more tasks are scheduled on the dying executor in the short time window of asking for the executor loss reason. Was there a resolution to that discussion?

The discussion was concluded that "I should do something about it". Let's not merge it until we have addressed that. I don't know if I'll have the bandwidth to do a second refactor though. Does anyone want to take a stab at it?
I was working to get everything in the current architecture cleaned up and sound first. I think the second refactor that would be required is more or less orthogonal to most of the code that is touched so far.

re: the new race, I think it's fine to do it as a separate change. It's separate enough from the code being changed here that trying to tack both changes here would be confusing.
I'm a little conflicted about whether to wait for that to be changed before we can merge this; on one hand, this change introduces that race. On the other, the failure mode the race introduces is not much different from the existing one - when executors are preempted, tasks will fail and eventually fail the app. It might make it slightly worse, but we're already in a bad spot today, and this change is an improvement in the right direction.

Merged build triggered.

Merged build started.

Test build #41985 has started for   PR 8007 at commit e62ee14.

Test build #41985 has finished for   PR 8007 at commit e62ee14.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/41985/
Test PASSed.

I'm still a little nervous that automated testing wasn't really done / isn't really possible for this code, at least on an end-to-end level. Anyone have any suggestions for that?

@mccheah @vanzin I think for this patch we can proceed without fixing the race condition. It may actually turn out to be something that's important to fix, since the window is not only 1 RT between the driver and the AM, but also the scheduling delay between each interval on the AM. At the very least we should file a JIRA and add a TODO comment here that links to that JIRA.
Would you have time to address that issue as well? It may actually not be super hard to fix. We just need some kind of blacklist that we add to when onDisconnected is called. Then we'll remove the executors from the blacklist as soon as the future finishes.
As for this patch, once you address the comments I think it's basically ready for merge.

Latest change does everything sans the unit test addition with my question about it.

Merged build triggered.

Merged build started.

Test build #42226 has started for   PR 8007 at commit bd5068a.

Test build #42226 has finished for   PR 8007 at commit bd5068a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42226/
Test PASSed.

Merged build finished. Test PASSed.

Merged build triggered.

Merged build started.

OK, let's merge this and fix the follow-up issues separately. Thanks for your work @mccheah.

Test build #42286 has started for   PR 8007 at commit ee6fffa.

Test build #42286 has finished for   PR 8007 at commit ee6fffa.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExecutorLostFailure(execId: String, isNormalExit: Boolean = false)
class ExecutorLossReason(val message: String) extends Serializable
case class ExecutorExited(exitCode: Int, isNormalExit: Boolean, reason: String)
  case class RemoveExecutor(executorId: String, reason: ExecutorLossReason)
  case class GetExecutorLossReason(executorId: String) extends CoarseGrainedClusterMessage

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42286/
Test FAILed.

authors: AmplabJenkins SparkQA andrewor14 markgrover mccheah vanzin


text:
WW-4877: adds missing header with license

Each file must have a header with license see. Also this PR will enable checking missing headers on each build.
Resolves WW-4877 #

LGTM 💯

Great .... let it be ;-)

source: gharchive/pull-request

authors: lukaszlenart sdutry

repo: apache/struts

license_type: permissive

license_source: github-api


text:
The problem with tf.compat.v1.disable_v2_behavior

Description
When importing some tensorflow_v2 modules, the subsequent TF2 functions cannot work properly.
I found out that some modules call the tensorflow.disable_v2_behavior function when they are imported. It will disable the subsequent TF2 behaviors.
cd submarine-sdk/pysubmarine/submarine

$ grep -rnH 'disable_v2_behavior'
ml/tensorflow_v2/layers/core.py:20:tf.compat.v1.disable_v2_behavior()
utils/tf_utils_v2.py:26:    tf.compat.v1.disable_v2_behavior()
utils/tf_utils_v2.py:57:        tf.compat.v1.disable_v2_behavior()

Furthermore, without the tensorflow.disable_v2_behavior, the tests still work as usual. Maybe the disable_v2_behavior is not necessary to the tf2 modules.
In summary, We should remove the tf.compat.v1.disable_v2_behavior statements to avoid the similar problem.
How to reproduce the TF2 error
Create Python venv.
cd submarine-sdk/pysubmarine
python3 -m venv .venv
source .venv/bin/activate
python3 -m pip install -e .[tf2]

Create demo.py and copy the following Python code into it.
# demo.py
import tensorflow as tf

from submarine.ml.tensorflow_v2.model import DeepFM   # call `tf.compat.v1.disable_v2_behavior` function

class LinearNNModel(tf.keras.Model):
    def __init__(self):
        super().__init__()
        self.dense1 = tf.keras.layers.Dense(1, activation=tf.nn.relu)  # One in and one out

    def call(self, x):
        y_pred = self.dense1(x)
        return y_pred

model = LinearNNModel()
input_arr = tf.random.uniform((1, 5))
model(input_arr)

model.save('sample-model')

Running the Python code. the error message shows that the TF2 function cannot work properly.
$ python3 demo.py

...
Traceback (most recent call last):
  File "/home/tom/huang06/submarine/submarine-sdk/pysubmarine/demo.py", line 21, in <module>
    model.save('sample-model')
  File "/home/tom/huang06/submarine/submarine-sdk/pysubmarine/.venv/lib/python3.10/site-packages/keras/utils/traceback_utils.py", line 67, in error_handler
    raise e.with_traceback(filtered_tb) from None
  File "/home/tom/huang06/submarine/submarine-sdk/pysubmarine/.venv/lib/python3.10/site-packages/keras/saving/save.py", line 142, in save_model
    raise NotImplementedError(
NotImplementedError: Saving the model to HDF5 format requires the model to be a Functional model or a Sequential model. It does not work for subclassed models, because such models are defined via the body of a Python method, which isn't safely serializable. Consider saving to the Tensorflow SavedModel format (by setting save_format="tf") or using `save_weights`.

@featherchen
Hi~ There are some questions about the tf part of the codes that we need your help to answer, please. Hope you can help us when you have time.
Thanks!

source: gharchive/issue

repo: apache/submarine


text:
Cannot enter more than one character at a time in simple filter text box

In charts, when adding a simple filter, you cannot type more than one character at a time into the value text box. After you type one character, the text box is deselected, and you must click the text box again in order to type another character.
This occurs if the column + the operator is selected, and occurs for all operators except IN and NOT IN.
Expected results
Text box should not be deselected after one character is entered.
Actual results
https://user-images.githubusercontent.com/14146019/123159110-77920680-d421-11eb-82f6-f0c564de6c36.mov
How to reproduce the bug

Go to chart explore
Add a filter
Select a column and select "equals" as the operator
Try to type into the textbox

Environment
(please complete the following information):

superset version: up-to-date with master as of 2021-06-22
python version: python --version 3.8
node.js version: node -v v14.16.1

Checklist
Make sure to follow these steps before submitting your issue - thank you!

[X ] I have checked the superset logs for python stacktraces and included it here as text if there are any.
[ X] I have reproduced the issue with at least the latest released version of superset.
[ X] I have checked the issue tracker for the same issue and I haven't found one similar.

@serenajiang, Thanks for the reporting. PR merged

authors: serenajiang zhaoyongjie

repo: apache/superset


text:
NULL does not get shown when grouping by a column

Make sure these boxes are checked before submitting your issue - thank you!

[ ] I have checked the superset logs for python stacktraces and included it here as text if any
[x] I have reproduced the issue with at least the latest released version of superset
[x] I have checked the issue tracker for the same issue and I haven't found one similar

Superset version
"version": "0.15.4"
Expected results
Note that by setting series limit to 0, we're able to get expected result, which includes num_candidate_type is NULL (0 on legend)

Actual results
Actual result does not include NULL values for num_candidates_type.  Looking at the query, there is an extra join added on num_candidates_type and as a result, when num_candidates_type IS NULL, those rows get dropped.

Steps to reproduce

for Group By, select a column that has NULL values
count(*) for metric
Select Line time series
Time Column should be one that contains string dates in the format "2017-01-01"
Select day as the Time Grain
Select some time range

@bkyryliuk FYI

Notice: this issue has been closed because it has been inactive for 443 days. Feel free to comment and request for this issue to be reopened.

This is still a valid issue in November 2019. I did a pie chart and NULL values are not included in the chart.

Hi - I've continued to notice this issue in Dec 2020. I made a line chart with a group-by and the NULL values aren't represented as data points. When I poke at the query created - I see this
SELECT "git_wrapper_version" AS "git_wrapper_version",
       date_trunc('day', CAST("day" AS TIMESTAMP)) AS "__timestamp",
       COUNT(*) AS "count"
FROM "default"."pal_git_wrapper_metrics"
WHERE "day" >= '2020-12-09'
  AND "day" < '2020-12-16'
GROUP BY "git_wrapper_version",
         date_trunc('day', CAST("day" AS TIMESTAMP))
ORDER BY "count" DESC
LIMIT 50000;

But I don't see a line in the resulting graph for when git_wrapper_version = NULL.
Note that the following slightly different query w/o a GROUPBY does show a line. The moment you add git_wrapper_version to the group-by the null line goes away.
SELECT date_trunc('day', CAST("day" AS TIMESTAMP)) AS "__timestamp",
       COUNT(*) AS "count"
FROM "default"."pal_git_wrapper_metrics"
WHERE "day" >= '2020-12-09'
  AND "day" < '2020-12-16'
  AND "git_wrapper_version" IS NULL
GROUP BY date_trunc('day', CAST("day" AS TIMESTAMP))
ORDER BY "count" DESC
LIMIT 50000;

@nipunn1313 please open a new ticket for this if you can confirm that it's still happening on master branch or the latest stable release (the original ticket is referring to version 0.15.4 which is very old).

It is still happening when grouped by 2 values and both of them are nulls, will try to find a way to repro on the example datasets

@bkyryliuk would you mind opening a new ticket for this issue with details e.g. db and charts that are being affected. Thanks!

authors: BartlomiejSkwira bkyryliuk dimakern junlincc mistercrunch nipunn1313 villebro


text:
Upgrading from 2.0.0 to 2.0.1rc2 causes an error when setting owner and roles on dashboard

Spin up an 2.0.0 Superset instalation in docker
Create Dashboard.
Edit setting on dashboard and you are able to set owner, and roles on dashboard
Upgrade to 2.0.1rc2
Edit setting on dashboard

Expected results
In owner and roles you should be able to select user / role
Actual results
An error appears in drop down: Cannot read properties of undefined (reading 'active'). This happens both in Owner and Role
Screenshots

Environment
(please complete the following information):

browser type and version: Chrome 106.0.5249.119
superset version: 2.0.1
python version: 3.8.12
node.js version: node -v
any feature flags active: {"ALERT_REPORTS": True, "DASHBOARD_CROSS_FILTERS": True, "DASHBOARD_RBAC": True, "THUMBNAILS" : True, "THUMBNAILS_SQLA_LISTENERS": True, "ENABLE_TEMPLATE_PROCESSING": True, "ENABLE_TEMPLATE_REMOVE_
FILTERS": True, "ENABLE_SCHEDULED_EMAIL_REPORTS": True, "EMAIL_NOTIFICATIONS": True, "EMBEDDED_SUPERSET": True}

Checklist
Make sure to follow these steps before submitting your issue - thank you!

[X] I have checked the superset logs for python stacktraces and included it here as text if there are any.
[X] I have reproduced the issue with at least the latest released version of superset.
[X] I have checked the issue tracker for the same issue and I haven't found one similar.

Additional context

We are also facing the same issue while adding owners to dashboards.

Faced this error too. Found out that browser gets data for users and roles if you're clicking on the field. Also this happens only if you're editing dashboard properties.

just for the record, I tried this migration some weeks ago .. and the entire application (frontend) wasn't work
maybe it was something about custom html pages or maybe some configurations .. or something in the structure of package is different than the previous one .. and my own conda package (based on the conda-forge version) for that was not working properly

also have the same issue

Is this still an issue with the newer 2.0.1 release candidates? rc4 is up for testing, and rc5 is en route.

yes still open. on my instance I get this error message in the newest rc5

CC @eschutho @AAfghahi

I think @eric-briscoe is hot on the trail of this one.

This should be fixed for 2.0.1. Closing the issue, but feel to reopen if you find it again while testing.

The error is gone now with 2.0.1-rc6 but we are still unable to change the dashboard ownership. If we try to save the changes nothing happens. I've checked the backend logs and looks like we are getting a 400 error on the API call to update the dashboard:
[16/Dec/2022:11:07:08 +0000] "PUT /api/v1/dashboard/6 HTTP/1.1" 400 75 "https://<ENDPOINT_REDACTED>/dashboard/list/?pageIndex=0&sortColumn=changed_on_delta_humanized&sortOrder=desc&viewMode=table" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/108.0.0.0 Safari/537.36"

2.0.1rc6 seems to fix this. Saving owner and roles does work in my test environment.

Unfortunatelly, after upgrade superset to 2.0.1, it still cant update owners for dashboard:(

Yeah, we'll take another look. Thanks for reporting. cc @eric-briscoe

FYI I can't replicate this bug in 2.0.1, I can add/remove owners and roles on both existing (from 2.0.0) and new dashboards.

@eschutho is this issue fully resolved. Is it safe to upgrade to 2.0.1 with dashboard_rbac enabled?

@mdeshmu Does this issue only happen when you have dashboard_rbac enabled?

authors: DiogoReisPinto HetviTA eschutho mattrei mdeshmu oleg-savko rusackas sfirke shhhowtime xmnlab ziggekatten


text:
test: Adds tests to TableLoader component

SUMMARY

Adds tests to TableLoader component
Moves TableLoader component to its own folder
Fixes a bug in Profile where toasts weren't being rendered. Found by the news tests! 🙌🏼
Creates a new helper function to mock the store

@rusackas @junlincc
TEST PLAN
1 - Execute ButtonGroup tests
2 - All tests should pass
ADDITIONAL INFORMATION

[ ] Has associated issue:
[ ] Changes UI
[ ] Requires DB Migration.
[ ] Confirm DB Migration upgrade and downgrade tested.
[ ] Introduces new feature or API
[ ] Removes existing feature or API

Codecov Report

Merging #13299 (4491981) into master (99a0c8a) will decrease coverage by 19.07%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #13299       +/-   ##
===========================================
- Coverage   77.15%   58.08%   -19.08%
===========================================
  Files         866      478      -388
  Lines       45005    16000    -29005
  Branches     5357     4129     -1228
===========================================
- Hits        34723     9293    -25430
+ Misses      10159     6707     -3452
+ Partials      123        0      -123

Flag
Coverage Δ

cypress
58.08% <ø> (-0.44%)
:arrow_down:

javascript
?

python
?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

...et-frontend/src/messageToasts/components/Toast.tsx
71.42% <ø> (-16.38%)
:arrow_down:

...uperset-frontend/src/dashboard/util/dnd-reorder.js
0.00% <0.00%> (-100.00%)
:arrow_down:

...rset-frontend/src/dashboard/util/getEmptyLayout.js
0.00% <0.00%> (-100.00%)
:arrow_down:

...dashboard/components/resizable/ResizableHandle.jsx
0.00% <0.00%> (-100.00%)
:arrow_down:

...tersConfigModal/Footer/CancelConfirmationAlert.tsx
0.00% <0.00%> (-100.00%)
:arrow_down:

...ConfigModal/FiltersConfigForm/FilterScope/state.ts
0.00% <0.00%> (-100.00%)
:arrow_down:

...odal/FiltersConfigForm/FilterScope/ScopingTree.tsx
6.25% <0.00%> (-93.75%)
:arrow_down:

.../src/dashboard/util/getFilterScopeFromNodesTree.js
0.00% <0.00%> (-93.48%)
:arrow_down:

...src/dashboard/components/gridComponents/Header.jsx
10.52% <0.00%> (-86.85%)
:arrow_down:

superset-frontend/src/components/IconTooltip.tsx
13.33% <0.00%> (-86.67%)
:arrow_down:

... and 728 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99a0c8a...4491981. Read the comment docs.

All suggestions applied. Thanks @rusackas!

authors: codecov-io michael-s-molina


text:
chore: allow machine auth to generate CSRF token

The celery task get permissions through machine_auth.
The 'cache-warmup' task needs to call the '/api/v1/chart/warm_up_cache' endpoint .However, this endpoint is non GET requests and is therefore protected by CSRF.
Hence, when machine_auth retrieves the auth cookies, it also needs to generate a CSRF token.
This ensures that the Celery task can operate successfully.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Configure the cache-warmup task.
The Celery task successfully executed the fetch_url action.

ADDITIONAL INFORMATION

[ ] Has associated issue:
[ ] Required feature flags:
[ ] Changes UI
[ ] Includes DB Migration (follow approval process in SIP-59)

[ ] Migration is atomic, supports rollback & is backwards-compatible
[ ] Confirm DB migration upgrade and downgrade tested
[ ] Runtime estimates and downtime expectations provided

[ ] Introduces new feature or API
[ ] Removes existing feature or API

It's related to #28706


text:
[SYSTEML-1758] added cbind and rbind for GPU

Ping @niketanpansare

Refer to this link for build results (access rights to CI server needed):
https://sparktc.ibmcloud.com/jenkins/job/SystemML-PullRequestBuilder/1738/

Refer to this link for build results (access rights to CI server needed):
https://sparktc.ibmcloud.com/jenkins/job/SystemML-PullRequestBuilder/1739/

LGTM ... thanks @nakul02

Running the tests again.

Refer to this link for build results (access rights to CI server needed):
https://sparktc.ibmcloud.com/jenkins/job/SystemML-PullRequestBuilder/1751/

Tests passed. I shall merge.

authors: akchinSTC nakul02 niketanpansare

repo: apache/systemml


text:
deliveryserviceserver POST endpoint can add servers with mismatching CDN from Delivery Service

I'm submitting a ...

[X] bug report
[ ] new feature / enhancement request
[ ] improvement request (usability, performance, tech debt, etc.)
[ ] other

Traffic Control components affected ...

[ ] CDN in a Box
[ ] Documentation
[ ] Grove
[ ] Traffic Control Client
[ ] Traffic Monitor
[X] Traffic Ops
[ ] Traffic Ops ORT
[ ] Traffic Portal
[ ] Traffic Router
[ ] Traffic Stats
[ ] Traffic Vault
[ ] unknown

Current behavior:

A POST request to /#!/deliveryserviceserver with a server that belongs to a different CDN than the CDN associated to the delivery service is allowed, returns status code 200 OK
Expected / new behavior:

Should not be allowed to add server with mismatching CDN
Should return status code 400 Bad Request
Minimal reproduction of the problem with instructions:

Anything else:

Server could also be of any Type and be added, does not have to be an EDGE or ORG.
Traffic Portal handles this by calling the deliveryservices/{{ID}}/servers/eligible endpoint in order to populate the dropdown list of servers the user can add.

Duplicate of #3756

repo: apache/trafficcontrol


text:
Remove file containing only logic for APIv1

What does this PR (Pull Request) do?

[x] This PR is not related to any Issue

This PR removes logic for handling the now-removed /cachegroups/{{ID}}/unassigned_parameters endpoint that was only in APIv1. CGP in general needs to stick around until we get rid of APIv3, but this is now unused.
Which Traffic Control components are affected by this PR?

Traffic Ops

What is the best way to verify this PR?
Make sure all the tests still pass.
The following criteria are ALL met by this PR

[x] Tests are unnecessary
[x] Documentation is unnecessary
[x] An update to CHANGELOG.md is not necessary
[x] This PR includes any and all required license headers
[x] This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

#6030 seems related enough to #6028 for them to be combined. Any reason not to combine them?

Supposedly the difference is that this is removing things from Traffic Ops while #6028 is removing things from lib/go-tc, but it is true that #6028 still does touch Traffic Ops to change some type names from RiakPing to TrafficVaultPing. They can probably be combined if you want - but then you need to promise to review the result!

Deal

Closed in favor of #6028


text:
Replace TsBuffer.h with std::string_view, ts::TextView or ts::MemSpan

Add capabilities missing (if any) to TextView and MemSpan.

OK changed title and description based on your comment.

https://github.com/apache/trafficserver/issues/4509

repo: apache/trafficserver


text:
ESI:combo_handler: Make maximum file count runtime configurable.

Oath has been running with a local patch to bump the max file count up. Rather than hand patch every release, the plugin should take an optional argument to set the number.

[approve ci linux]

[approve ci autest]

👍  on 100 default file count and 4KB default length

authors: SolidWallOfCode djcarlin


text:
s3_auth autest: convert from gold file to file contains

This makes the autest more resilient against different curl versions by checking just for return status and content length for test pass instead of using a gold file.

Cherry-picked to v9.2.x

authors: traeak zwoop


text:
[CI] Rev ci-cpu image to include latest PRs

I'm just raising this as a request for the ci-cpu image to be updated, to include latest PRs updating dependencies. Specifically, I'm interested to get #8514 into the image.
As a way to simplify the work required by someone with access to upload tlcpack images, I've already tested images based on my ongoing work with tlcpackstaging. Here is the job in which all tests pass with an updated set of images: https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/ci-docker-staging/146/pipeline
So, my understanding of the work needed here is:
docker pull tlcpackstaging/ci_cpu:20210815-225913-1a95f9bd0
docker tag tlcpackstaging/ci_cpu:20210815-225913-1a95f9bd0 tlcpack/ci-cpu:v0.76   # already bumped by 0.01
docker push tlcpack/ci-cpu:v0.76

Once this is done, we can create a PR to Rev the image version and than make it permanent.
So, the tasks:

[ ] pull, retag, push docker images using commands above (needs to be done by someone in the list below)
[ ] submit a PR updating the ci-cpu version on Jenkins (one the one above is done, I can do that myself)

I have no permission to upload images myself, so I'm adding here the people I think have access to do so: @tqchen @areusch @jroesch @zhiics @masahi.
cc @grant-arm @u99127

@leandron I am currently executing the push, will ping back when its done.

@leandron its currently uploading to dockerhub

That is completed now. Closing the issue. Thanks @jroesch.

authors: jroesch leandron

repo: apache/tvm


text:
Fixed pylint issues after moving to venv in ci_lint docker

PR https://github.com/apache/tvm/pull/12663 introduced
virtual environment in ci_lint docker image. At present, this
is not updated on docker hub. In the old docker image prior
to the above PR, pylint picked up a different version of python
that didn't catch the issues fixed in this commit.
cc @leandron @areusch

oops, thanks @ashutosh-arm !

authors: areusch ashutosh-arm


text:
[Relay] Add set_attrs_type registry to broadcast_to op

It seems that ir text is not parsable without set_attrs_type registry.
%2 = broadcast_to(%1, shape=[128, 1, 28]);

Good catch. Seems that this code path is not covered by any tests. Would that be possible that we can add a test to cover the usage of this relay op?

authors: shingjan wrongtest-intellif


text:
[YUNIKORN-1595] postTaskAllocated uses same event details for different things

What is this PR for?
The volume bind and pod to node bind failures use the same event content:
https://github.com/apache/yunikorn-k8shim/blob/master/pkg/cache/task.go#L383
and
https://github.com/apache/yunikorn-k8shim/blob/master/pkg/cache/task.go#L396
That is confusing, we should change the description text.
What type of PR is it?

[ ] - Bug Fix
[x] - Improvement
[ ] - Feature
[ ] - Documentation
[ ] - Hot Fix
[ ] - Refactoring

Todos

[ ] - Task

What is the Jira issue?

Open an issue on Jira https://issues.apache.org/jira/browse/YUNIKORN-1595
Put link here, and add [YUNIKORN-Jira number] in PR title, eg. [YUNIKORN-2] Gang scheduling interface parameters

How should this be tested?
Screenshots (if appropriate)
Questions:

[ ] - The licenses files need update.
[ ] - There is breaking changes for older versions.
[ ] - It needs documentation.

+1

authors: pbacsko zhuqi-lucas


text:
[ZEPPELIN-987][WIP] Enable user to secure interpreter setting, credentials and configurations info

What is this PR for?
For some user case, people might want to hide Interpreter Setting, Credentials and Configurations information to other users (who are defined in conf/shiro.ini). So I added
#/api/interpreter/** = authc, roles[admin]
#/api/configurations/** = authc, roles[admin]
#/api/credential/** = authc, roles[admin]

below the  [urls]  section.
This issue was originally suggested in Zeppelin user mailing list by @TomNorden
What type of PR is it?
Improvement | Documentation
Todos

[x] - Add interpreter, credential and configuration url to conf/shiro.ini
[x] - Update shiroauthentication.md for this change
[x] - Redirect to home with ngToast error message when status is 401
[ ] - Rebase after #1100 merged and add error message to Credential menu as well

What is the Jira issue?
ZEPPELIN-987
How should this be tested?

Apply this patch and restart Zeppelin
Login with admin and password1
Go to interpreter, credential and configuration tab -> You can see all of the information in each tabs
Logout -> Login again with user1 and password2
Go to interpreter, credential and configuration tab -> In this time, you can't see all of the information in each tabs

Screenshots (if appropriate)

When you login with user1 (doesn't have permission to see the interpreter, credential and cofiguration info)

interpreter menu

configuration menu

shiroauthentication.md

Questions:

Does the licenses files need update? No
Is there breaking changes for older versions? No
Does this needs documentation? Yes, so I updated.

@AhyoungRyu  is this still a work in progress ?
Is there a way in which we can go away with seeing all of these three munu i.e. Interpreter setting, Credentials and Configurations if user is not authorize  to ?
Otherwise LGTM.

@prabhjyotsingh No, I'm just waiting some reviews. I'm not sure that i understood your comment correctly,

Is there a way in which we can go away with seeing all of these three menu i.e. Interpreter setting, Credentials and Configurations if user is not authorize to ?

I just wanted to make even if some users are already defined in [users] section, the other user ( maybe admin) can hide the three information. This is the scope of this PR :)

I was thinking if all of these 3 Interpreter, Credentials, and Configuration menu can be hidden, like the way they are hidden in case of user not loggedin.

@prabhjyotsingh Yeah it makes sense. It would be better. Let me figure out it then :)

@AhyoungRyu If you can come to an implementation that fits @prabhjyotsingh good idea, it would be great if you could make it generic, with a configurable list of functions that reflect in to the menu.

@echarles Yes. Thanks for your suggestion ! :)

nice！

@AhyoungRyu Still working on this?
Could you rebase the branch with master?

@prabhjyotsingh Sorry for my late response. Since i think they need to know the existence of those menus(Interpretes, Configurations and Credentials), I added ngToast error message instead of totally hiding the menus. See the updated gif images in the PR description. What do you think?

Sure this will work. Thanks for the fix.

@prabhjyotsingh Thanks for your feedback. I addressed it in the last commit :)

LGTM!

@prabhjyotsingh i'll rebase after #1100 merged and add the alert message to credential page as well :)

@prabhjyotsingh Since #1100 was merged into master, I updated credential.controler.js.

Tested on both firefox and chrome. Works well. 👍

Merging this if no more discussion.

authors: AhyoungRyu corneadoug cquptEthan echarles prabhjyotsingh

repo: apache/zeppelin

license_source: bigquery


text:
🛑 Element is down

In f6b7262, Element (https://element.interhop.org) was down:

HTTP code: 502
Response time: 789 ms

Resolved: Element is back up in dbe5f8e after 5 minutes.

license: MIT


text:
CAS for multiple clients of same name on multiple instances

We have two applications (abc and def) were developed in Struts2 and integrated with CAS server 3.2 for SSO, deployed on multiple hosts (IPs). That deployment architecture diagram is below. SSO was working fine with below deployment and there is no issue.
https://i.stack.imgur.com/xAVB9.png
We had deployed the same two CAS clients (abc and def) with multiple instances (tomcat with ports 8080 and 8081) on same host. Please see below deployment architecture diagram for this. With this SSO is not working fine here single sign on working fine but when user logout from abc application (its running on 8081 port of Host2) then session expire request will goes to def application (its running on 8080 port of Host2). With this user is not log out (session is not expired) from def application (its running on 8081 port of Host2).
https://i.stack.imgur.com/hmyqW.png
CAS Server sends logout command to all other apps. Here CAS server is sending logout command all apps (abc and def). But its sending logout command to abc on 8081 and def on 8080. But actually it should be def on 8081.
May be this is stupid question I too do't know. How to resolve this issue. Any one please help me in this. In above two scenarios URL is same http://domain.in/abc/login.do or http://domain.in/def/login.do

 Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.2 out of 3 committers have signed the CLA.:white_check_mark: astohn:white_check_mark: mmoayyed:x: dependabot[bot]You have signed the CLA already but the status is still pending? Let us recheck it.

repo: apereo/cas


text:
cors settings not being applied

Prerequisites

[x] I am running the latest version. (up upgrade)
[x] I searched to see if the issue already exists.
[x] I inspected the verbose debug output with the -v, --verbose flag.
[x] Are you an Up Pro subscriber?

Description
I do not think cors is being set. It was previously working for me. I am a subscriber to up pro but currently I am just using the base version.
Steps to Reproduce
I think this would be the quickest way you could verify my claim.
Using this example
https://github.com/apex/up-examples/blob/master/oss/node-graphql/up.json
up config results in this:
 "cors": {
    "allowed_origins": null,
    "allowed_methods": null,
    "allowed_headers": null,
    "exposed_headers": null,
    "allow_credentials": false,
    "max_int": 0,
    "debug": true
  },

Which does not match the defaults listed here https://up.docs.apex.sh/#configuration.cross_origin_resource_sharing
If there isn't a problem with the config, could you let me know where in AWS does cors get set? In the API Gateway? I want to go look at the settings myself to help me debug.
Thanks!
Love Up?
Please consider signing up for Up Pro (https://up.docs.apex.sh/#guides.subscribing_to_up_pro) or donating via https://opencollective.com/apex-up/donate.
Slack
Join us on Slack https://chat.apex.sh/

Ahh those defaults in the output are a bit misleading, I didn't catch that but the Go middleware for CORS implements the defaults (instead of inside Up) so they don't appear there.
When you deploy with debug: true do you see any CORS related logs? They might help pin-point why it's not working for a request, may have to allow more header fields.

@tj Thanks for your quick reply. Sorry for my slow one. Debug true helped. You were right I think it was an authentication header change that was not part of the default set.
For anyone who comes across this is what worked for me:
  "cors": {
    "allowed_origins": [
      "https://staging.mydomain.com",
      "https://mydomain.com"
    ],
    "allowed_methods": [
      "HEAD",
      "GET",
      "POST",
      "PUT",
      "PATCH",
      "DELETE",
      "OPTIONS"
    ],
    "allowed_headers": ["*"],
    "allow_credentials": true
  },

authors: tj vespertilian

repo: apex/up


text:
Can't export CSV getting errror "i.split is not a function" when series name is an array

Description
Steps to Reproduce

Open sandbox
click toolbox
click Download CSV

Expected Behavior
Downloads CSV
Actual Behavior
Error in console: "i.split is not a function"
Screenshots

Reproduction Link
https://codesandbox.io/s/bitter-browser-5z3qc8

I'm having the same error with this:
https://codepen.io/andreabrioschi/pen/PoRNXKr

@ordago Why are you setting the name of the series to an array? It should be a string. The series name is not expecting an array.
@AndreFra96 the name is expecting a string, not a number.

@brianlagunas  I remember seeing in the docs somewhere. Using an array for series whose name were too long would place the second item on the array in a new line.

@ordago I was unable to find any code that handled the name as an array. If you can find that doc, I would appreciate it. It's possible is was a feature that wasn't implemented fully.

hey @brianlagunas, maybe I mixed series names with categories
https://apexcharts.com/docs/multiline-text-and-line-breaks-in-axes-labels/

@ordago yeah, it looks like it. Glad we figured it out.

authors: AndreFra96 brianlagunas ordago


text:
Improve the visibility of mnemonic in generate-mnemonic command

Slack discussion
Burak: I think the nicest way would be to print the mnemonic in red

I don't know what I was thinking, red on dark is pretty difficult to read. Maybe we can utilize some emojis somehow.

I'm not sure about emojis. Also not sure about any text styling (boldness, color, etc.). Users would most likely want to copy that mnemonic out and we want to avoid it being copied with color codes or something similar.
I would suggest going with something like:
This mnemonic is created locally on your machine using "ethers.Wallet.createRandom" under the hood.
Make sure to back it up securely, e.g., by writing it down on a piece of paper:

####################################### MNEMONIC #######################################
##                                                                                ##
##    jealous anxiety dust play custom vintage member place shove cereal style wing   ##
##                                                                                ##
####################################### MNEMONIC #######################################

The Airnode address for this mnemonic is: 0x2346D6F5019232a92e56F25e4DBB5B585e9250D1
The Airnode xpub for this mnemonic is: xpub6CfpG6iVDNEtTdjMKM135bky5ycrXG347kShBUVLoJUxjLCsSjkh3c6R7a4v15Y6arEDQTR8acL9bsQGQsWk3UM2VJr7XJWMRZrDcEGNFk3

I too am a fan of ASCII art. This works very well imo.

I pulled the mnemonic-visibility branch

Why pull that branch, when you can just test this from master ?

That said, as there was a discussion regarding the ease of copying, I'd much rather see it like this:

That is a good point 👍, but I don't think it's that big of a deal, but leaving @amarthadan to decide.

You couldn't have pulled mnemonic-visibility branch as it was deleted.
Was it, though?

I don't see any advantage in your suggestion.
Again, copying from the console: you won't accidentally copy and hash/whitespace character.

That said, master produces the same result, i.e. no change. Please tell me what I am supposed to do now.

@kolenic-martin What exactly did you do? It works on master for me. Have you rebuild the project?
I see you're running yarn airnode-admin generate-mnemonic. To be honest I didn't know that works :smile: But yeah, that looks for node_modules/.bin/airnode-admin binary and that is only "refreshed" when you do the build. Or, you can run yarn cli generate-mnemonic from the packages/airnode-admin directory (that one will work without rebuilding).

just a side note:
It's better if @kolenic-martin keeps testing using yarn airnode-admin instead of the yarn cli because the first pattern can catch issues with wrong package.json binary path (a.k.a the validator issue in 0.7.1)

But yeah, that looks for node_modules/.bin/airnode-admin binary and that is only "refreshed" when you do the build. Or, you can run yarn cli generate-mnemonic from the packages/airnode-admin directory (that one will work without rebuilding).

Yup, rebuilding the project actually helped. But tbh on standard console screen size (80x24 IIRC) it looks bad, you might want to be under that limit. Look for yourself:

I don't think we need to honor the length character. User can just increase the size of the terminal and the general readability of the thing is more important.

Ok, I'm reopening this to change the format to be without a border for easier copying.

@amarthadan let me know when this is testable

@kolenic-martin Already merged 🙂

Sorry for the delay; I tested this in the morning, it's all good now. The whitespace/# removal makes it definitely a bit more practical.

authors: Siegrift amarthadan bbenligiray kolenic-martin


text:
Allow files to be selected for update in .apibuilder config files

Second part of #21

@mchimirev @mbryzek now it should be fixed in .apibuilder files too (backward compatible)

@mbryzek thoughts?

FYI - have updated locally and running with out issue - can you merge?

released 0.1.10 with this change

authors: gheine mbryzek

repo: apicollective/apibuilder-cli


text:
chore: Check whether version in PR is already released

We have a recurring issue that you often forget to bump the package version number in a PR, and then when you merge it to master, the package publication fails because the version you're trying to publish already exists.
This adds a check to CI which detects these issues early and fails the CI, so you can't merge those PRs without bumping the package version number.
It gets skipped if the title of the PR is docs: ..., because such PRs don't trigger package publication when merged.

Yup, once I merge it here and see that it works, I will copy it there too.

repo: apify/apify-sdk-python


text:
maxRequestsPerCrawl should restore its counter after migration

Currently the counter is reset and therefore long running actors will not stop when desired request count is reached.

It should be possible to get the number of crawled items from the RequestList too.

Fixed in https://github.com/apifytech/apify-js/pull/258

authors: jancurn mnmkng

repo: apifytech/apify-js


text:
Remove old travis files

Since we are using codeship I'm just removing this. I'm also checking if the CI pipeline is working or not since it didn't seem working so good in #228.

Is this ready?

Oh sorry I was gonna close this after I was done with my testing.

I'll open up a new one for removing travis once I find out what's going on with codeship.

authors: koddsson kristjanmik

repo: apis-is/apis

license: mit


text:
Conditional Lock Regions Cause Code Generation Bug

The LockOpElimination pass removes unnecessary lock operations and those that can cause code generation bugs.
It fails to remove lock region modifications when they are acquired and released conditionally within a single cycle.
E.g.,
if (x) {
  start(rf);
  acquire(rf);
  end(rf);
}

The main reason is that we don't look inside the generated conditional commands to remove these. This causes an error when trying to generate the BSV code (since it tries to write a register twice in the same cycle).

I believe this will be fixed in the PR #43

repo: apl-cornell/PDL


text:
i got failed to resolve

Error:(34, 13) Failed to resolve: com.github.apl-devs:appintro:v4.2.1
Show in FileShow in Project Structure dialog
i m using
maven { url 'https://jitpack.io' }
compile 'com.github.apl-devs:appintro:v4.2.1'

Error:(34, 12) Failed to resolve: com.github.apl-devs:appintro:v4.2.1
Show in File
Show in Project Structure dialog
i m using
maven { url 'https://jitpack.io' }
compile 'com.github.apl-devs:appintro:v4.2.1'

repositories {
----------
maven { url 'https://jitpack.io' }
}
}
this is soved

@iotwebplanet Where are you adding the maven declaration for JitPack?
@vivekaris Are you @iotwebplanet or has your particular issue being solved?

@vivekaris i am still getting this error adding repositories {
maven { url 'https://jitpack.io' }
}
}

This should solve your problem:
https://stackoverflow.com/a/41529064/2353588

In short Use latest android studio with latest gradle

authors: avluis iotwebplanet jbhingradiya shareefhiasat vivekaris willblaschko

repo: apl-devs/AppIntro


text:
Appintro show for first time

How to display Appintro for once , when Application first run?

https://github.com/apl-devs/AppIntro/wiki/How-to-Use#show-the-intro-once

authors: Andrew-Quebe hamedgol


text:
docs: add some document to show how to build when use windows system

Is your feature request related to a problem? Please describe.
When user want to build apolloconfig in windows os, he/she need to use scripts/build.bat or use Git Bash.
Describe the solution you'd like
We can add some document in faq, development to tell user how to do it.
Now there are some document in https://www.apolloconfig.com/#/zh/faq/common-issues-in-deployment-and-development-phase , but can we add more about it?
This will be more friendly to newcomer.

Normally the users don't need to manually build apollo. Speaking of building apollo in windows, is there some specific issues the user would encounter?

Everything is well when use build.bat in windows.

repo: apolloconfig/apollo


text:
Auto Updating Spring Placeholder Values

Auto Updating Spring Placeholder Values
1. Features
Spring placeholder values will now be auto updated when new configs are released in Apollo.
1.1 @Value Placeholders
Full support is provided for:

@Value placeholders annotated on fields
@Value placeholders annotated on methods
@Value placeholders with Spring Expression Language

For example, the following 3 fields can all be auto updated when the corresponding config is changed:
public class TestJavaConfigBean {

  @Value("${timeout:100}")
  private int timeout;
  private int batch;
  @Value("#{new java.text.SimpleDateFormat('${dateFormat}').parse('${dateProperty}')}")
  private Date someDate;

  @Value("${batch:200}")
  public void setBatch(int batch) {
    this.batch = batch;
  }

  public int getTimeout() {
    return timeout;
  }

  public int getBatch() {
    return batch;
  }

  public Date getSomeDate() {
    return someDate;
  }
}

@Configuration
@EnableApolloConfig
public class AppConfig {
  @Bean
  TestJavaConfigBean testJavaConfigBean() {
    return new TestJavaConfigBean();
  }
}

Sample Configurations:
timeout=1000
batch=200
dateFormat=yyyy-MM-dd HH:mm:ss.SSS
dateProperty=2018-02-23 20:01:02.123

We also provide limited support for nested placeholders.
For example, with the following configurations and program:

batch equals to normalBatch at first, so the initial nestedProperty equals to 1000
When batch is changed to specialBatch, then the nestedProperty field will be changed to 5000
However, if we don't change batch config and change normalBatch to 5000 instead, then the nestedProperty won't be auto updated

batch=normalBatch
normalBatch=1000
specialBatch=5000

public class TestNestedPropertyBean {

  @Value("${${batch}}")
  private int nestedProperty;

  public int getNestedProperty() {
    return nestedProperty;
  }
}

1.2 XML Placeholders
Full support is provided for placeholders for property field.
For example, the following 3 fields can all be auto updated when the corresponding config is changed:
public class TestXmlBean {
  private int timeout;
  private int batch;
  private Date someDate;

  public void setTimeout(int timeout) {
    this.timeout = timeout;
  }

  public int getTimeout() {
    return timeout;
  }

  public void setBatch(int batch) {
    this.batch = batch;
  }

  public int getBatch() {
    return batch;
  }

  public void setSomeDate(Date someDate) {
    this.someDate = someDate;
  }

  public Date getSomeDate() {
    return someDate;
  }
}

Xml Config:
<apollo:config/>

<bean class="com.ctrip.framework.apollo.spring.demo.TestXmlBean">
    <property name="timeout" value="${timeout:100}"/>
    <property name="batch" value="${batch:200}"/>
    <property name="someDate" value="#{new java.text.SimpleDateFormat('${dateFormat}').parse('${dateProperty}')}"/>
</bean>

Sample Configurations:
timeout=1000
batch=200
dateFormat=yyyy-MM-dd HH:mm:ss.SSS
dateProperty=2018-02-23 20:01:02.123

We also provide limited support for nested placeholders, the same as @Value placeholders.
2. Configuration
This auto update feature is turned on by default, you could turn off this feature by:

Specify apollo.autoUpdateInjectedSpringProperties=false in System Properties
For example, you may pass -Dapollo.autoUpdateInjectedSpringProperties=false when starting the Java program

Specify apollo.autoUpdateInjectedSpringProperties=false in META-INF/app.properties
For example,
app.id=SampleApp
apollo.autoUpdateInjectedSpringProperties=false

If apollo.autoUpdateInjectedSpringProperties is specified in both System Properties and META-INF/app.properties, then the one specified in System Properties wins.

Coverage increased (+1.4%) to 51.829% when pulling 3efd4a33a1be5425e96cc3052f141d3fd3fab903 on nobodyiam:auto-update-value-merge into 7edfb1aa8c06311202a013fe77e8e1c892638e2a on ctripcorp:master.

Codecov Report

Merging #972 into master will increase coverage by 1.36%.
The diff coverage is 90.61%.

@@             Coverage Diff              @@
##             master     #972      +/-   ##
============================================
+ Coverage     46.91%   48.28%   +1.36%
- Complexity     1554     1650      +96
============================================
  Files           354      360       +6
  Lines          9755    10062     +307
  Branches        968     1012      +44
============================================
+ Hits           4577     4858     +281
- Misses         4835     4847      +12
- Partials        343      357      +14

Impacted Files
Coverage Δ
Complexity Δ

.../apollo/spring/property/SpringValueDefinition.java
100% <100%> (ø)
4 <4> (?)

...ork/apollo/spring/config/ConfigPropertySource.java
100% <100%> (ø)
6 <1> (+1)
:arrow_up:

...pring/boot/ApolloSpringApplicationRunListener.java
86.66% <100%> (+0.95%)
9 <1> (ø)
:arrow_down:

...apollo/spring/config/PropertySourcesProcessor.java
100% <100%> (ø)
12 <0> (ø)
:arrow_down:

...va/com/ctrip/framework/apollo/util/ConfigUtil.java
82.92% <100%> (+1.34%)
38 <4> (+4)
:arrow_up:

...pollo/spring/annotation/ApolloConfigRegistrar.java
100% <100%> (ø)
2 <0> (ø)
:arrow_down:

...llo/spring/config/ConfigPropertySourceFactory.java
100% <100%> (ø)
3 <3> (?)

.../framework/apollo/spring/property/SpringValue.java
100% <100%> (ø)
15 <15> (?)

.../spring/config/ConfigPropertySourcesProcessor.java
100% <100%> (ø)
3 <1> (+1)
:arrow_up:

...ip/framework/apollo/internals/DefaultInjector.java
69.23% <100%> (+2.56%)
3 <0> (ø)
:arrow_down:

... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7edfb1a...3efd4a3. Read the comment docs.

Great job!

Perfect!

nice

spring 注入的方式还是不支持热配置，要用apollo自带的那种方式才行

嗯！！

您好，您的邮件我已收到


text:
Question: documentation

I've been struggling with documentation myself. The information is split between README file and docs folder and neither of them are complete.
I like how the iOS repo is structured:
https://github.com/apollographql/apollo-ios
Just putting the crucial info into README with a link to the docs. Then docs will be the source of truth.

If you all agree, I can start with copying some info into docs. When docs is more complete, then we can move some info from README and clean it.

Sounds good to me

Do you mean moving to a docs folder hosted on github or hosting the docs on www.apollographql.com ?
I actually like to have github as the source of truth. As a newcomer to a project, I only look for gradle artifacts dependencies and minimal example. If this could stay on the main README, that'll be great. Advanced topics (caching, snapshots, plugin configuration, etc...) can (and certainly could) be moved to sub pages.

Closing due to inactivity.

Why, it is just 2 months 😔
Currently this is a big problem for people learning GraphQL Android. There is no source of truth.

authors: digitalbuddha martinbonnin sav007 tasomaniac

repo: apollographql/apollo-android


text:
input-ctor-compiler-plugin

Use case
To avoid all the Optional madness in input types, leverage some compiler plugin. Kind of like Kotlin default parameters except they would wrap into Optional
Generated code using @InputConstructor (naming for grabs):
const val Absent: Any? = null
class SomeInput @InputConstructor constructor(a: Int? = Absent, b: String? = Absent) {
   val a: Optional<Int?>
   val b: Optional<String?>
}

What the compiler generates:
private constructor(a: Optional<Int?>, b: Optional<String?>, bitField: Int) { // Actual constructor with bitField containing the arguments that are set
   // Initialization code written by the plugin
   val a: Optional<Int?> = if (bitField.and(1)) Optional.present(a) else Optional.Absent
   val b: Optional<String?> if (bitField.and(2)) Optional.present(a) else Optional.Absent
}

Dropping in favor of generateInputBuilders (https://github.com/apollographql/apollo-kotlin/pull/5146)
generateInputBuilders is now available in 4.0.0-alpha.3-SNAPSHOT. If needed we can backport it to 3.x

repo: apollographql/apollo-kotlin


text:
Playlist primary_color field doesn't exist in Spotify Web API

We have a primary_color field included as part of the Playlist, PlaylistSimplified and PlaylistTrack types but I can't find it anywhere in the Spotify Web API.

Hey @mabuyo 👋
Unfortunately its an undocumented field. I added the types based on the JSON response from their API directly.
Here is a sample playlist directly from their API:

Response JSON

{
  collaborative: false,
  description: '',
  external_urls: {
    spotify: 'https://open.spotify.com/playlist/7ztqQ9YOM05hRje13ZUg7J'
  },
  followers: { href: null, total: 0 },
  href: 'https://api.spotify.com/v1/playlists/7ztqQ9YOM05hRje13ZUg7J',
  id: '7ztqQ9YOM05hRje13ZUg7J',
  images: [
    {
      height: 640,
      url: 'https://i.scdn.co/image/ab67616d0000b273f72cd22e67f690f7a2f95829',
      width: 640
    }
  ],
  name: 'TEST - Mix of songs and episodes',
  owner: {
    display_name: 'Jerel Miller',
    external_urls: {
      spotify: 'https://open.spotify.com/user/1228247189'
    },
    href: 'https://api.spotify.com/v1/users/1228247189',
    id: '1228247189',
    type: 'user',
    uri: 'spotify:user:1228247189'
  },
  primary_color: null,
  public: false,
  snapshot_id: 'NCwwYTZlZjVhNzE1YjBmOWExZmVhNDkxNDQwMWM2ZDI3MGY5ZWI3MTJj',
  tracks: {
    href: 'https://api.spotify.com/v1/playlists/7ztqQ9YOM05hRje13ZUg7J/tracks?offset=0&limit=100&locale=en-US',
    items: [
      {
        added_at: '2022-12-15T00:00:12Z',
        added_by: {
          external_urls: {
            spotify: 'https://open.spotify.com/user/1228247189'
          },
          href: 'https://api.spotify.com/v1/users/1228247189',
          id: '1228247189',
          type: 'user',
          uri: 'spotify:user:1228247189'
        },
        is_local: false,
        primary_color: null,
        track: {
          album: {
            album_type: 'single',
            artists: [
              {
                external_urls: {
                  spotify: 'https://open.spotify.com/artist/2vKiJjsgjgqIECUyYeIVvO'
                },
                href: 'https://api.spotify.com/v1/artists/2vKiJjsgjgqIECUyYeIVvO',
                id: '2vKiJjsgjgqIECUyYeIVvO',
                name: 'Crown The Empire',
                type: 'artist',
                uri: 'spotify:artist:2vKiJjsgjgqIECUyYeIVvO'
              }
            ],
            available_markets: [],
            external_urls: {
              spotify: 'https://open.spotify.com/album/3kJDQqifVrTnTEOj1alM13'
            },
            href: 'https://api.spotify.com/v1/albums/3kJDQqifVrTnTEOj1alM13',
            id: '3kJDQqifVrTnTEOj1alM13',
            images: [
              {
                height: 640,
                url: 'https://i.scdn.co/image/ab67616d0000b27349a4637ad0514e78085b4ddc',
                width: 640
              },
              {
                height: 300,
                url: 'https://i.scdn.co/image/ab67616d00001e0249a4637ad0514e78085b4ddc',
                width: 300
              },
              {
                height: 64,
                url: 'https://i.scdn.co/image/ab67616d0000485149a4637ad0514e78085b4ddc',
                width: 64
              }
            ],
            name: 'Immortalize',
            release_date: '2022-12-02',
            release_date_precision: 'day',
            total_tracks: 3,
            type: 'album',
            uri: 'spotify:album:3kJDQqifVrTnTEOj1alM13'
          },
          artists: [
            {
              external_urls: {
                spotify: 'https://open.spotify.com/artist/2vKiJjsgjgqIECUyYeIVvO'
              },
              href: 'https://api.spotify.com/v1/artists/2vKiJjsgjgqIECUyYeIVvO',
              id: '2vKiJjsgjgqIECUyYeIVvO',
              name: 'Crown The Empire',
              type: 'artist',
              uri: 'spotify:artist:2vKiJjsgjgqIECUyYeIVvO'
            }
          ],
          available_markets: [],
          disc_number: 1,
          duration_ms: 215154,
          episode: false,
          explicit: false,
          external_ids: { isrc: 'QMRSZ2200976' },
          external_urls: {
            spotify: 'https://open.spotify.com/track/1AR0d9urAEX4a1WdTzmbKz'
          },
          href: 'https://api.spotify.com/v1/tracks/1AR0d9urAEX4a1WdTzmbKz',
          id: '1AR0d9urAEX4a1WdTzmbKz',
          is_local: false,
          name: 'Immortalize',
          popularity: 29,
          preview_url: null,
          track: true,
          track_number: 1,
          type: 'track',
          uri: 'spotify:track:1AR0d9urAEX4a1WdTzmbKz'
        },
        video_thumbnail: { url: null }
      },
      {
        added_at: '2022-12-15T00:00:18Z',
        added_by: {
          external_urls: {
            spotify: 'https://open.spotify.com/user/1228247189'
          },
          href: 'https://api.spotify.com/v1/users/1228247189',
          id: '1228247189',
          type: 'user',
          uri: 'spotify:user:1228247189'
        },
        is_local: false,
        primary_color: null,
        track: {
          album: {
            album_type: 'compilation',
            artists: [
              {
                external_urls: {
                  spotify: 'https://open.spotify.com/show/3kUJiCxp9sy2JfheQ6hIb0'
                },
                href: 'https://api.spotify.com/v1/shows/3kUJiCxp9sy2JfheQ6hIb0',
                id: '3kUJiCxp9sy2JfheQ6hIb0',
                name: 'Broncos Country Tonight',
                type: 'show',
                uri: 'spotify:show:3kUJiCxp9sy2JfheQ6hIb0'
              }
            ],
            available_markets: [
              'AD', 'AE', 'AG', 'AL', 'AM', 'AO', 'AR', 'AT',
              'AU', 'AZ', 'BA', 'BB', 'BE', 'BF', 'BG', 'BH',
              'BI', 'BJ', 'BN', 'BO', 'BR', 'BS', 'BT', 'BW',
              'BZ', 'CA', 'CH', 'CI', 'CL', 'CM', 'CO', 'CR',
              'CV', 'CW', 'CY', 'CZ', 'DE', 'DJ', 'DK', 'DM',
              'DO', 'DZ', 'EC', 'EE', 'EG', 'ES', 'FI', 'FJ',
              'FM', 'FR', 'GA', 'GB', 'GD', 'GE', 'GH', 'GM',
              'GN', 'GQ', 'GR', 'GT', 'GW', 'GY', 'HK', 'HN',
              'HR', 'HT', 'HU', 'ID', 'IE', 'IL', 'IN', 'IS',
              'IT', 'JM', 'JO', 'JP', 'KE', 'KH', 'KI', 'KM',
              'KN', 'KR', 'KW', 'LA', 'LB', 'LC', 'LI', 'LR',
              'LS', 'LT', 'LU', 'LV', 'MA', 'MC', 'ME', 'MG',
              'MH', 'MK', 'ML', 'MN', 'MO', 'MR', 'MT', 'MU',
              'MV', 'MW', 'MX', 'MY', 'MZ', 'NA', 'NE', 'NG',
              'NI', 'NL', 'NO', 'NP', 'NR', 'NZ', 'OM', 'PA',
              'PE', 'PG', 'PH', 'PL', 'PS', 'PT', 'PW', 'PY',
              'QA', 'RO', 'RS', 'RW', 'SA', 'SB', 'SC', 'SE',
              'SG', 'SI', 'SK', 'SL', 'SM', 'SN', 'SR', 'ST',
              'SV', 'SZ', 'TD', 'TG', 'TH', 'TL', 'TN', 'TO',
              'TR', 'TT', 'TV', 'TW', 'TZ', 'UA', 'US', 'UY',
              'UZ', 'VC', 'VN', 'VU', 'WS', 'XK', 'ZA', 'ZM',
              'ZW'
            ],
            external_urls: {
              spotify: 'https://open.spotify.com/show/3kUJiCxp9sy2JfheQ6hIb0'
            },
            href: 'https://api.spotify.com/v1/shows/3kUJiCxp9sy2JfheQ6hIb0',
            id: '3kUJiCxp9sy2JfheQ6hIb0',
            images: [
              {
                height: 640,
                url: 'https://i.scdn.co/image/ab6765630000ba8a16b9a523e035becd31cecf3c',
                width: 640
              },
              {
                height: 300,
                url: 'https://i.scdn.co/image/ab67656300005f1f16b9a523e035becd31cecf3c',
                width: 300
              },
              {
                height: 64,
                url: 'https://i.scdn.co/image/ab6765630000f68d16b9a523e035becd31cecf3c',
                width: 64
              }
            ],
            is_playable: false,
            name: 'Broncos Country Tonight',
            release_date: null,
            release_date_precision: null,
            restrictions: { reason: 'product' },
            total_tracks: 1,
            type: 'show',
            uri: 'spotify:show:3kUJiCxp9sy2JfheQ6hIb0'
          },
          artists: [
            {
              external_urls: {
                spotify: 'https://open.spotify.com/show/3kUJiCxp9sy2JfheQ6hIb0'
              },
              href: 'https://api.spotify.com/v1/shows/3kUJiCxp9sy2JfheQ6hIb0',
              id: '3kUJiCxp9sy2JfheQ6hIb0',
              name: 'Broncos Country Tonight',
              type: 'show',
              uri: 'spotify:show:3kUJiCxp9sy2JfheQ6hIb0'
            }
          ],
          available_markets: [
            'AD', 'AE', 'AG', 'AL', 'AM', 'AO', 'AR', 'AT',
            'AU', 'AZ', 'BA', 'BB', 'BE', 'BF', 'BG', 'BH',
            'BI', 'BJ', 'BN', 'BO', 'BR', 'BS', 'BT', 'BW',
            'BZ', 'CA', 'CH', 'CI', 'CL', 'CM', 'CO', 'CR',
            'CV', 'CW', 'CY', 'CZ', 'DE', 'DJ', 'DK', 'DM',
            'DO', 'DZ', 'EC', 'EE', 'EG', 'ES', 'FI', 'FJ',
            'FM', 'FR', 'GA', 'GB', 'GD', 'GE', 'GH', 'GM',
            'GN', 'GQ', 'GR', 'GT', 'GW', 'GY', 'HK', 'HN',
            'HR', 'HT', 'HU', 'ID', 'IE', 'IL', 'IN', 'IS',
            'IT', 'JM', 'JO', 'JP', 'KE', 'KH', 'KI', 'KM',
            'KN', 'KR', 'KW', 'LA', 'LB', 'LC', 'LI', 'LR',
            'LS', 'LT', 'LU', 'LV', 'MA', 'MC', 'ME', 'MG',
            'MH', 'MK', 'ML', 'MN', 'MO', 'MR', 'MT', 'MU',
            'MV', 'MW', 'MX', 'MY', 'MZ', 'NA', 'NE', 'NG',
            'NI', 'NL', 'NO', 'NP', 'NR', 'NZ', 'OM', 'PA',
            'PE', 'PG', 'PH', 'PL', 'PS', 'PT', 'PW', 'PY',
            'QA', 'RO', 'RS', 'RW', 'SA', 'SB', 'SC', 'SE',
            'SG', 'SI', 'SK', 'SL', 'SM', 'SN', 'SR', 'ST',
            'SV', 'SZ', 'TD', 'TG', 'TH', 'TL', 'TN', 'TO',
            'TR', 'TT', 'TV', 'TW', 'TZ', 'UA', 'US', 'UY',
            'UZ', 'VC', 'VN', 'VU', 'WS', 'XK', 'ZA', 'ZM',
            'ZW'
          ],
          disc_number: 0,
          duration_ms: 1457711,
          episode: false,
          explicit: false,
          external_ids: {
            spotify: 'https://open.spotify.com/episode/1CwDjPKXfQGDlitW2gaPYY'
          },
          external_urls: {
            spotify: 'https://open.spotify.com/episode/1CwDjPKXfQGDlitW2gaPYY'
          },
          href: 'https://api.spotify.com/v1/episodes/1CwDjPKXfQGDlitW2gaPYY',
          id: '1CwDjPKXfQGDlitW2gaPYY',
          is_local: false,
          is_playable: false,
          name: '12-08-22 Bonus Hour of Broncos Country Tonight',
          popularity: 0,
          preview_url: 'https://podz-content.spotifycdn.com/audio/clips/7AUcRFQCmS3LJhxVOctnF8/clip_1235400_1281100.mp3',
          track: true,
          track_number: 0,
          type: 'episode',
          uri: 'spotify:episode:1CwDjPKXfQGDlitW2gaPYY'
        },
        video_thumbnail: { url: null }
      }
    ],
    limit: 100,
    next: null,
    offset: 0,
    previous: null,
    total: 2
  },
  type: 'playlist',
  uri: 'spotify:playlist:7ztqQ9YOM05hRje13ZUg7J'
}

You'll note the primary_color field in here. To be honest, I don't think its ever set to anything other than null or an empty string. Since its undocumented, its probably safe to remove from our types anyways.
Not sure if you're looking to set a background based on the image of the playlist, but if you want the trick I used, check out the following code:

Parse the "vibrant" color from an image and return its swatch: https://github.com/apollographql/spotify-showcase/blob/main/client/src/utils/image.ts
Add a local resolver on the Image type so I can query for a vibrantColor as a client-only field: https://github.com/apollographql/spotify-showcase/blob/main/client/src/apollo/resolvers.ts
An example of where I query this for the playlist page: https://github.com/apollographql/spotify-showcase/blob/0eeac35bf8bb2cb95da095a676320dcf3a187cfa/client/src/routes/playlists/playlist.tsx#L45

Hope this helps! Let me know if you have any questions.

authors: jerelmiller mabuyo

repo: apollographql/spotify-showcase