[SPARK-23243][Core] Fix RDD.repartition() data correctness issue

What changes were proposed in this pull request?
The RDD repartition uses a round-robin way to distribute data, thus there may be data correctness issue if only a sub-set of partitions are recomputed on fetch failure and the input data sequence is not deterministic.
The RDD data type may be not sortable, so we cannot resolve the whole issue by insert a local sort before shuffle (while we shall still provide that solution as a optional choice for those RDDs with sortable data type). The approach proposed in this PR is to always recompute all the partitions before shuffle on fetch failure, thus we don't rely on certain input data sequence.
Please note that with the feather on you may observe a higher risk of job falling due to reach max consequence stage failure limit, esp. for large jobs running on a big cluster.
How was this patch tested?
TBD

Add unit tests.
Integration test on my own cluster.
Benchmark to ensure no performance regression when fetch failure doesn't happen.

Test build #92526 has started for PR 21698 at commit b14cd4b.

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/testing-k8s-prb-make-spark-distribution-unified/620/
Test PASSed.

Test build #92526 has finished for PR 21698 at commit b14cd4b.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

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/92526/
Test FAILed.

retest this please

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/testing-k8s-prb-make-spark-distribution-unified/621/
Test PASSed.

Test build #92527 has started for PR 21698 at commit b14cd4b.

Test build #92527 has finished for PR 21698 at commit b14cd4b.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

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/92527/
Test FAILed.

Thanks @cloud-fan @viirya comments addressed :)

Test build #92616 has started for PR 21698 at commit 52e76e9.

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/testing-k8s-prb-make-spark-distribution-unified/679/
Test PASSed.

Test build #92616 has finished for PR 21698 at commit 52e76e9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

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/92616/
Test PASSed.

I did not go over the PR itself in detail, but the proposal sounds very expensive - particularly given the cascading costs involved.
Also, I am not sure why we are special case'ing only coalasce/repartition here : any closure which is depending on ordering of tuples is bound to fail - for example, RDD.zip* variants, sampling in ML etc will suffer from same issue.
Either we fix shuffle itself to become deterministic (which I am not sure if we can do efficiently), or we could simply document this issue with coalasce/other relevant api - so that users do a sort when applicable : when they deem the additional cost is required to be borne.
Note that in a lot of cases, this is not an issue - for example when reading from external data stores, checkpointed data, persisted data, etc : which typically are reasons why coalasce gets used a lot (to minimize number of partitions).

Thank you for your comments @mridulm !
We focus on resolving the RDD.repartition() correctness issue here in this PR, because it is most commonly used, and that we can still address the RDD.zip* issue using the similar approach. At first I was worried that the changes may be huge and trying to address the correctness issue for multiple operations may make it difficult to backport the PR. But now it turns out that the PR didn't change that much code, so maybe I can consider include the RDD.zip* fix in this PR too.
Since you are also deeply involved in the related discussion on the correctness issue caused by non-deterministic input for shuffle, you may also agree that there is actually no easy way to both guarantee correctness and don't cause serious performance drop-off. I have to insist that correctness always goes beyond performance concerns, and that we shall not expect users to always remember they may hit a known correctness bug in case of some use patterns.
As for the proposed solution, there are actually two ways to follow: Either you insert a local sort before a shuffle repartition (that's how we fixed the DataFrame.repartition()), or you always retry the whole stage with repartition on FetchFailure. The problem with the local-sort solution is that, it can't fix all the problems for RDD (since the data type of an RDD can be not sortable, and it's hard to construct a sorting for a generic type), also it can make the time consumption of repartition() increases by 3X ~ 5X. By applying the approach proposed in this PR, the performance shall keep the same in case no FetchFailure happens, and it shall works well for DataFrames as well as for RDDs.
I have to admit that if you have a big query running on a huge cluster, and the tasks can easily hit FetchFailure issues, then you may see the job takes more time to finish (or even fall due to reach max consequence stage failure limit). But again, your big query may be producing wrong result without a patch, and I have to say that is even more unacceptable :( . As for the cascading cost, you are right, it makes things worse, and I don't have good advice for that issue.

IMO RDD as a distributed data set, it should not guarantee any record order unless you sort it. So user functions and Spark internal functions should not expect a specific record order.
However, the round robin partitioner violates it. If the record order changes during retry, we may get wrong result. That's why we should fix repartition but not something else.
I agree with @mridulm that this may introduce a big perf penalty. But when a repartition task fails, we should pay the cost to get the corrected result, instead of producing wrong result and asking users to deal with it themselves.
I feel this is a better solution than the sort one. We should only pay the cost when we really need to, i.e. when the failure happens.

@cloud-fan The difference would be between a (user) defined record order (global sort or local sort) and expectation of repeatable record order on recomputation.
It might also be a good idea to explore how other frameworks handle this.

However, the round robin partitione(following with a shuffle) violates it.

This is is not limited to repartition : any closure which depends on input order has the same effect - repartition/coalesce is one instance of this issue - I gave a few examples from spark itself; and I am sure there are other examples from spark and user code.
It is possible this issue was initially identified via repartition - but modeling the solution only for one manifestation of the issue ignores all others and leaves them unfixed.

For zip, it's hard to define what result is "corrected", given the fact that RDD is unordered. I think sample should be similar.
repartition is special because we may change the number of outputs if this bug is triggered, which is obviously wrong result.

@cloud-fan We should not look at a particular stage in isolation, but rather what happens when there are failures in the middle of a job with multiple shuffle stages - and zip is one of the internal stages.
A synthetic example:
rdd1.zip(rdd2).map(v => (computeKey(v._1, v._2), computeValue(v._1, v._2))).groupByKey().map().save()
If relative ordering of rdd1 or rdd2 changes, the computed key would change - and we end up with data loss if some of the tasks in save have already completed.

A synthetic example:
rdd1.zip(rdd2).map(v => (computeKey(v._1, v._2), computeValue(v._1, v._2))).groupByKey().map().save()

The above example may create some different output when retrying a subset of all the tasks. But I may not call it a data loss or data correctness issue. Let's image you run the query twice, each with different ordering of rdd1 and rdd2, each run shall produce different outputs (even different # of output rows). The result produced by retrying a subset of all the tasks is still valid, it actually correspond to another input data representation,  though not the same as the initial input.
Now I tend to believe there will not be data loss or data correctness issue, as long as you don't spread input data across partitions in a round robin way (or, in a way that is not related to the data itself), because on task retry you are guaranteed that all input data are covered (each row get recomputed exactly once, though maybe in different order).

@jiangxb1987 Different number of output rows is due to data loss - it is not another valid run.
A complete re-execution of the job in this case could result in a different ordering, but consistent output characterstics (number of rows for example).

@mridulm you provided a good example to show the indeterminacy of zip.
rdd1.zip(rdd2).map(v => (computeKey(v._1, v._2), computeValue(v._1, v._2))).groupByKey().map().save()

If rdd1 or rdd2 is unordered, then any result can be treated as a corrected result of this query. If rdd1 and rdd2 is ordered, we don't have a problem.
On the other hand, rdd.repartition is very clear about what is the corrected result. No matter rdd is ordered or not, repartition will not add/remove/update the existing records.
Basically user builds an RDD DAG and Spark should produce a result to meet user's expectation. For zip the user's expectation is: the # of output records of each partition is min(# of record of the corresponding partition in rdd1, # of record of the corresponding partition in rdd2). For map, the expectation can be very vague, zip(...).map(...) can produce any result. For repartition, the expectation is clear: not add/remove/update the existing records.
That's why we should fix repartition because it violates the user's expectation.

@cloud-fan There is no ambiguity in output of map - one record in, one record out.
In case of zip, as you said, number of output records is min of both.
Given this, there is no ambiguity in cardinality of zip().map() - I think @jiangxb1987's point was that which two tuples from rdd1 and rdd2 get zip'ed together can be arbitrary : and I agree about that.
Note that the problem I surfaced above will cause data loss even after the proposed fix in this PR by @jiangxb1987 btw.

Given this, there is no ambiguity in cardinality of zip().map() ... which two tuples from rdd1 and rdd2 get zip'ed together can be arbitrary : and I agree about that.

yes, but the following .groupByKey().map() has ambiguity in cardinality because the tulples get zipped can be arbitrary, isn't it?

IIUC the output produced by rdd1.zip(rdd2).map(v => (computeKey(v._1, v._2), computeValue(v._1, v._2))) shall always have the same cardinality, no matter how many tasks are retried, so where is the data loss issue?

@cloud-fan That depends on what the computeKey is doing - which is user defined. It can have different values, or it need not (again, depends on user data and closure being applied).

@jiangxb1987 data loss comes because a re-execution of zip might generate a key for which corresponding reducer has already finished.
Hence re-execution of stage will not result in subsequent child stage's reducer partition getting re-executed : resulting in data loss.

Taking a step back and analyzing the solution for the problem at hand.
There are three main issues with the proposal:

It does not solve the problem in a general manner.

I gave example of zip, sample - it applies to any order sensitive closure.

It does not fix the issue when a child stage has one or more completed tasks.

Even if we assume it is a specific fix for repartition/coalasce - even there it does not solve the problem and can cause data loss.

It causes performance regression to existing workaround.

The common workaround for this issue is to checkpoint + action or do a local/global sort (I believe sql does the latter now ?).
The proposal causes performance regression for these existing workarounds.

The corner case where the proposal works is if :
a) order sensitive stage has finished and
b) no task in child stage has finished fetching its shuffle input.
This is a fairly narrow subset, and why I dont believe the current approach helps.
Having said that, if it is possible to enhance the approach, that would be great !
This is a fairly nasty issue which hurts users, and typically people who are aware of the problem tend to always pay a performance cost to avoid the corner case.

I think all we discussed here is about promise and expectation, and your zip example seems is not an issue: according to the promise that Spark does not guarantee output order of unsorted RDD(task may see different order when it gets retired) and users will expect your zip example to return random values. The first thing we need to agree on is: this is not a general issue, it's only about repartition. I think it's pretty clear that users will not expect rdd.repartition(x)count to return a different result from rdd.count. cc @JoshRosen @zsxwing

It does not fix the issue when a child stage has one or more completed tasks.

The approach here is to recompute all the partitions of the child stage, the completed task will be ignored. Do I miss something here? If we can't fix the issue for repartition, this patch must be rejected. @jiangxb1987 please explain more about this fix and make sure it works.

It causes performance regression to existing workaround.

The perf regression only happens when users call repartition and have more shuffles after repartition, and some repartition tasks fail. That said, users only need to pay the perf penalty when they are going to have the wrong answer.

The common workaround for this issue is to checkpoint + action or do a local/global sort

Do you have any reference to the common workaround? It would be better to follow a common solution.

Actually I think @mridulm have a point here - if we only retry all the tasks for repartition/zip*, it's still possible that some tasks in a succeeding stage may have finished before retry, and outputs that shall goes to those tasks/partitions may be ignored and that causes data loss.
If we are going down the current way to fix the issue, we have to track all the (partially) finished succeeding stages of a stage, and when we retry a stage, we also retry all the succeeding stages. One typical scenario is that when you hit an ExecutorLost, and you have to retry a chain of finished stages.
However, this approach can still be useful, because if you don't hit a FetchFailure in your job containing multiple repartition()/zip* actions, you don't have to pay for the performance penalty (This is a major difference from the insert-local-sort approach). However, I now tend to add an extra flag to enable users to disable the approach (when they believe they are aware of the correctness issue and have better way to handle that - such as manually do checkpointing).
Actually you are not able to sort a RDD if the data type is not sortable, so you either retry the stage (and all the finished tasks from  succeeding stages), or users have to manually handle that. Given by that, I still think this approach can be useful, esp. for small jobs that are not likely to hit FetchFailure, or for complex jobs on small dataset and users care correctness more over performance. Note that, with an extra flag, users can always disable this approach, and the behavior/performance shall be exactly the same as previous versions.

Ah I see, then we need to change DAGScheduler a lot to fix it, which may not worth.
I still insist that repartition should be treated as a bug, but it's not easy to hit(when the input order is random) and very hard to fix. Maybe documenting it is the best we can do for now.

jumping in the middle on this discussion -- everybody has raised some great points.

My first takeaway from this is just how hard it can be to reason about this, because spark's exact semantics aren't really that clear.  repartition() has been in spark for a while and this issue went ignored; we're all experienced with spark and still have to think about it very carefully; imagine the difficulty for the average spark user!  I propose we add some documentation describing the actual fault-tolerant semantics, and what users can and can't do.  In addition to the ordering issue we're discussing here, another case I've seen users get wrong is with random data generators.

More specific points:

I agree with @mridulm about zip etc., not just repartition.  It took quite a while for me to see this.  The point isn't that you need a specific ordering of the output, its that if you zip [a, b] with [1,2], your output should never be [(a,1), (a,2)], which I think is possible in the current situation (though probably needs a significantly more complicated example).

I do think that repartition is probably the most serious case users will hit this, and I would still be open to consider a special case fix.

I see some discussion about making shuffles deterministic, but it proved to be very difficult.  Is there a prior discussion on this you can point me to?  Is it that even if you used fetch-to-disk and had the shuffle-fetch side read the map-output in a set order, you'd still have random variations in spills?

this might be a dumb idea -- since we only need to do this sort on RDDs post shuffle, we know the records have been serialized to bytes, and you can always compare bytes.  Has this been considered at all?  I realize it would be a mess in the current code, as those serialized bytes from the shuffle aren't exposed where you'd want to put this, but I want to consider whether it would even be correct.  and an easier to implement approach, though horribly inefficient, would be to just serialize again.

The proposed solution gives you correctness at the price of a very expensive recomputation.  Another option would be to just fail-fast -- on a fetch-failure in repartition, fail the entire job.  You could even expose a configuration to allow both your current approach or this.

We could add logic to detect whether even an order-dependent operation was safe to retry -- eg. repartition just after reading from hdfs or after a checkpoint can be done as it is now.  Each stage would need to know this based on extra properties of all the RDDs it was defined from.   (I'm not sure this is what you mean, @cloud-fan by "Ah I see, then we need to change DAGScheduler a lot to fix it, which may not worth." -- if you meant something else, can you explain?)

Honestly I consider this bug so serious I'd consider loudly warning from every api which suffers from this if we can't fix -- make them deprecated and log a warning.

I see some discussion about making shuffles deterministic, but it proved to be very difficult. Is there a prior discussion on this you can point me to? Is it that even if you used fetch-to-disk and had the shuffle-fetch side read the map-output in a set order, you'd still have random variations in spills?

Related discussion can be found https://github.com/apache/spark/pull/20414 . Also, let me list some of the scenarios that might generate non-deterministic row ordering below:
Random Shuffle Blocks Fetch
We randomize the ordering of block fetches on purpose, for avoiding all the nodes fetching from the same node at the same time. That means, we send out multiple concurrent pull requests, and the fetched blocks are processed in FIFO. Therefore, the row ordering of shuffle output are non-deterministic.
Shuffle Merge With Spills
The shuffle using Aggregator (for instance, combineByKey) uses ExternalAppendOnlyMap to combine the values. The ExternalAppendOnlyMap claims that it keeps the row orders, but it actually uses the hash to compare the elements (i.e., HashComparator). Even though the sort algorithm is stable, the map sizes can be different when the spilling happens. The requests for additional memory might be in different orders. The spilling could be non deterministic and thus the resulting order can still be non-deterministic.
Read From External Data Source
Some external data sources might generate different row ordering of outputs on different read request.

since we only need to do this sort on RDDs post shuffle

IIUC this is not the case in RDD.repartition(), see https://github.com/apache/spark/blob/94c67a76ec1fda908a671a47a2a1fa63b3ab1b06/core/src/main/scala/org/apache/spark/rdd/RDD.scala#L453~L461 , it requires the input rows are ordered then perform a round-robin style data transformation, so I don't see what we can do if the input data type is not sortable.

on a fetch-failure in repartition, fail the entire job

Currently I can't figure out a case that a customer may vote for this behavior change, esp. FetchFailure tends to occur more often on  long-running jobs on big datasets compared to interactive queries.

We could add logic to detect whether even an order-dependent operation was safe to retry -- eg. repartition just after reading from hdfs or after a checkpoint can be done as it is now. Each stage would need to know this based on extra properties of all the RDDs it was defined from.

This is something I'm also trying to figure out, that we shall enable users to tell Spark that an RDD will generate deterministic output, so you don't have to worry about data correctness issue over these RDDs. Please also note that actually checkpoint can not guarantee that you shall always get the same output on each read operation, because you may have executorLost, and then you have to recompute the partitions thus may fetch different data.

Honestly I consider this bug so serious I'd consider loudly warning from every api which suffers from this if we can't fix -- make them deprecated and log a warning.

We shall definitely update the comments, but shall we make the apis deprecated? I can't say I agree or disagree on this. I'm still trying to extend the current approach to allow data correctness, and the code changes shall be well flagged off. Maybe we can revisit the deprecated apis proposal after that?

checkpoint can not guarantee that you shall always get the same output ...

IIRC we can checkpoint to HDFS? Then it becomes reliable.

checkpoint can not guarantee that you shall always get the same output ...

IIRC we can checkpoint to HDFS? Then it becomes reliable.

Sure, thanks for clarify on that.

sorry I got bogged down in some other things, thanks for the responses:

on a fetch-failure in repartition, fail the entire job

Currently I can't figure out a case that a customer may vote for this behavior change, esp. FetchFailure tends to occur more often on long-running jobs on big datasets compared to interactive queries.

yeah maybe you're right.  I was thinking that maybe there comes a point where if you have one failure, you expect more failures on retries as well (in my experience, large shuffles often fail the first time when everything is getting fetched, but on subsequent retries they manage to succeed because the load is smaller).  It might be better to just not bother retrying.  But then again, there are situtations where retry is fine, and I guess users won't know which one to choose.

since we only need to do this sort on RDDs post shuffle

IIUC this is not the case in RDD.repartition(), see https://github.com/apache/spark/blob/94c67a76ec1fda908a671a47a2a1fa63b3ab1b06/core/src/main/scala/org/apache/spark/rdd/RDD.scala#L453~L461 , it requires the input rows are ordered then perform a round-robin style data transformation, so I don't see what we can do if the input data type is not sortable.

my point is that if you serialize the input (the Iterator[T] there), then there is a well-defined ordering based on the serialized bytes.  (I guess I'm assuming serialization is deterministic, I can't think of a case that isn't true.)  In general, you don't know that T is serializable, but after a shuffle you know it must be.  So that gives you a way to always deterministically order the input after a shuffle, though at a pretty serious performance penalty.  You could avoid the re-serialization overhead by pushing the sort down into ShuffleBlockFetcherIterator etc.  Maybe you could skip this if you detect checkpointing or something equivalent which eliminates the ordering dependency ... or maybe thats just not possible with the current apis.
thanks for the description of the problem with determinstic shuffle ordering.  The "Shuffle Merge With Spills" problem seems particularly hard to solve.

Sorry for coming in late on this, first I saw this was the other day.
Could someone perhaps summarize the discussions here and exactly when this happens and why? Checkpointing was mentioned to work around the issue, why?  Would be good to add those details to the jira anyway.
My initial reaction is this is very bad.  Any correctness issue we cause from handle failures is not something we should write off and expect the user to handle.
repartition seems to be the most obvious case and I know lots of people use it, although hopefully many are using the dataframe api) and we see fetch failures on large jobs all the time, so it seems really serious.
Trying to use a similar example as what is listed in jira SPARK-23207 with an RDD doesn't reproduce this:
import scala.sys.process._

import org.apache.spark.TaskContext
val res = sc.parallelize(0 to (1000000-1), 1).repartition(200).map { x =>
  x
}.repartition(200).map { x =>
  if (TaskContext.get.attemptNumber == 0 && TaskContext.get.partitionId < 2) {
    throw new Exception("pkill -f java".!!)
  }
  x
}
res.distinct().count()

@tgravescs its not guaranteed to reproduce with that.  IIUC, you need to do a repartition in the same stage that also does a shuffle-read, then have a fetch failure, and on recompute that stage needs to fetch shuffle data in a different order.  I think you probably need to make sure the fetches are remote to get a different order on a retry (local shuffle reads are deterministic, I think).
@jiangxb1987 has worked on this stopped?  I think there are still ideas for how to go forward on this, and its a really important fix.

I'm still working on this, hopefully I can update this PR by the end of this week. Sorry I didn't get back to this earlier because I was working on the barrier scheduling issue.

Actually I have been thinking about the issue and feel we can still go further with the current patch to fix the issue with child stages @mridulm mentioned above, that requires track all the preceding stages that can generate non-deterministic output for each active/finished stage, and also track the stages that have partially finished (some tasks in the stage have finished, while the rest still running or pending). In case of FetchFailure(or ExecutorLost/WorkerLost), if the affected stage can generate non-deterministic output, then clear all the shuffle files from that stage (to make sure we’ll retry all tasks for that stage), and also check all partially finished stages that are succeeding stages of the affected stage, also clear all the shuffle files of them (If a stage is succeeding stage of the affected stage but it has already finished successfully and no shuffle files are lost, then it’s fine we don’t need to retry it).
The above fix proposal requires more code refactoring of DAGScheduler, and it shall consume some memories to store additional informations (assume you have M active/finished stages, and N stages may generate non-deterministic output, then it shall take at most M * N Int values to store the stageIds). Normally N is by far smaller than the total number of stages, so I think the extra cost shall be acceptable.
Actually, you always have to pay a lot of extra effort on FetchFailure, this may increase the effort by several times, but shall be statistically fine considering most Spark jobs are short-running and don't hit FetchFailure quite often (The major advantage of this approach is that you don't pay for any penalty if you don't hit FetchFailure).

statistically fine considering most Spark jobs are short-running and don't hit FetchFailure quite often (The major advantage of this approach is that you don't pay for any penalty if you don't hit FetchFailure).

I don't think this is the right metric we should be considering.  The majority of jobs may be small, but (a) thats different than total compute time (one big job is equal to many small jobs) and (b) many users start small because they believe spark will scale well for them.
Fetch failures are way more common on big jobs.

More thoughts: what if the last step of the job is writing data out? Do we need to improve the OutputCoordinator to support canceling all the writing tasks? Shall we simplify the logic and just retry the entire job?
Or shall we go to the other direction and think about how to make RDD output deterministic? by sort or checkpoint.
Or shall we just deprecate these problematic operations like repartition and zip?

The above fix proposal requires more code refactoring of DAGScheduler, and it shall consume some memories to store additional informations (assume you have M active/finished stages, and N stages may generate non-deterministic output, then it shall take at most M * N Int values to store the stageIds). Normally N is by far smaller than the total number of stages, so I think the extra cost shall be acceptable.

I think its going to be really hard to determine N.  You'll have to upper-bound it by including any stage which has an arbitrary user function (eg. an rdd.map).  But in any case, I don't know if its changing the order of things at all, as spark already has to have enough info to rebuild lineage back to the last checkpoint.
I think @cloud-fan is right, with that proposal you might also need to do some special handling for output steps.
I'd like to restate my previous proposal, that you can sort an RDD deterministically post-shuffle, regardless of the row type, based on the serialized bytes.  So the fix would similar to SQL, though probably more expensive as its harder to know when you can skip that extra sort, and its more work to do the sort itself.
I also like ideas based on checkpointing

I also like ideas based on checkpointing

What if the user does't provide a distributed file system path? E.g., you can read from Kafka and write them back to Kafka and such workloads don't need a distributed file system in standalone mode.

IIUC streaming query always need to specify a checkpoint location?

What happened to the other pr proposal of just using the hashPartitioner?

It's not able to handle the use case when we use repartition() to shuffle skewed data, so we give up on that way.

Ok,  it seems like the proposal @squito had to sort on the binary/serialized data seems like at least a good short term solution.  any sorting is going to definitely add overhead but at least its not dataloss/corruption.    Did anyone see issues with that?
another solution would be to have another partitioner that somehow deals with the skew, not sure on the details of that though as it might need sampling or something else to work.

IIUC streaming query always need to specify a checkpoint location?

You can use a batch query to read and write Kafka :) My point is if the input and output data sources are not distributed file system, the user doesn't need to specify a file system location to checkpoint. In addition, if the user doesn't specify a checkpoint path, which path should we use?

thinking about the sorting thing again but I don't think it works unless you sort both the keys and the values themselves.
for instance lets say we have a groupby key which generates:
B = k1, { (a11, a12, a13), (a21, a22, a23), (a11, a12, a13)}
Then we do a distinct on it, you could get:
C =((a11, a12, a13) ,(a21, a22, a23))
But say you get fetch failures and you refetch the data.  Since the reducer can fetch it in any order during the shuffle,  B could be
B = k1, { (a21, a22, a23), (a11, a12, a13), (a11, a12, a13)}
and distinct you end up with
C =((a21, a22, a23), (a11, a12, a13))
If you were to just sort that C could be in a different place and then round robin the output, the above example of C would go to different partitions.
I'm not sure of a way to get around this without using the hash partitioner but still looking into it.  I need to look at the details about how the dataframe  pr solved this as well as I'm curious how it works with above example.

yeah, you'd have to sort the entire record.  I think originally it didn't seem like that would work, because you don't know that T is sortable for RDD[T].  But after a sort, you've got bytes, so you can at least sort the serialized bytes of T.  Note that you don't want to do the entire repartitioning based on that, since you won't get a good distribution for skewed data.  But it can give you a deterministic order as an additional sort, just within one partition.

We fixed the DataFrame repartition correctness issue by inserting a local sort before repartition, and feedback for this approach is generally negative because the performance of repartition() goes down significantly, and that even queries that don't have potential correctness issue also have to pay for the performance regression (eg. rdd.repartition(...).map(...).collect()). That's the major reason why we are trying to resolve the correctness issues in a different way.
I agree that correctness shall be allocated higher priority than performance, but things are not black or white, here we really care about both. We also want to guarantee that if you don't have correctness issue before hand, you are least affected by the proposed fix approach.
I'm currently working on a extended approach based on that proposed in this PR, that shall handle the cascading stages issue @mridulm mentioned above (rdd1.zip(rdd2).map(v => (computeKey(v._1, v._2), computeValue(v._1, v._2))).groupByKey().map().save()). Please also note that this actually don't implies we will retry more stages, it's true we will retry more tasks to ensure correctness on FetchFailure/ExecutorLost, but we won't retry more stages.
Having said that, IMO the best bet is to implement both approaches(the insert-local-sort one and the retry-all-tasks-on-failure one) and create a flag for each of them, so users may choose a approach based on different workload patterns, though it's also debatable which approach shall be enabled by default. Unfortunately we are not able to deliver them on 2.4, but I'm optimistic we may include them in 3.0 and of course backport them to all the active branches.

if you are looking at recomputing how are you going to handle if some tasks have already written output?  This was brought up by @cloud-fan above and I didn't see a response.  Some output formats have a task commit and then a job commit so it may work for those, but others might not have that.

Unfortunately we are not able to deliver them on 2.4, but I'm optimistic we may include them in 3.0 and of course backport them to all the active branches.

I really disagree with this.  We need to fix this in some way before 2.4 release.  If the sort way is a fix but performance regression we should do that as its at least fixed by default.  We have the config for people who are ok with possible corruption and just want the performance.  I wouldn't think its any worse then what is there for dataframes based on what you have said.
I totally understand if you won't have time to work on this but perhaps others will.
I plan on working on this now but would be nice for us to come up with a overall approach.
did anyone run benchmarks on the fix for dataframes?   I'm really curious what the real performance implications are.
Note that Apache PIG also had a similar issue withe the round robin partitioner and they removed it and used a hash value partitioner.   Spark is obviously different but the underlying issue is the same.  I would actually prefer to see us just use the hash partitioner if we can't find a better solution.  Our official docs I don't think says it repartitions evenly (http://spark.apache.org/docs/2.3.1/api/scala/index.html#org.apache.spark.rdd.RDD), but our programming guide does:
http://spark.apache.org/docs/latest/rdd-programming-guide.html
"
Reshuffle the data in the RDD randomly to create either more or fewer partitions and balance it across them. This always shuffles all data over the network.
"
So I could see the argument that says we can't change that behavior.

I really disagree with this.

I really agree with Tom. At this point, I think the working assumption should be that any 2.4.0 release candidate that doesn't deliver some fix for this issue will receive multiple -1 votes from PMC members.

@jiangxb1987  can you clarify if you are working on this still or if you won't have time for a bit?
@mridulm @zsxwing @cloud-fan  thoughts on @squito approach to "you can at least sort the serialized bytes of T"

"you can at least sort the serialized bytes of T"

I think this should work.

@tgravescs I'm still working on this but I would be glad if you can also work on the "sort the serialized bytes of T" approach, actually the retry-all-tasks approach seems more complex than I expected when it involves commit protocol (currently a task can be only committed once, so if you already have some tasks committed and then hit a ExecutorLost then retry-all-tasks won't work), so I hope we can have other approaches like "sort the serialized bytes of T" get merged into 2.4 release.
I'll post the benchmark result of DF.repartition() fix later.

I also think @tgravescs solution of using the HashPartitioner is an acceptable one, though as you've noted it doesn't deal w/ skew (which may be a lot of the existing use of repartition()).  I think we'd probably see a bunch of users complain that their jobs started crashing on upgrading 2.4 if thats the best we can offer, but IMO crash is way better than silent data loss.

@tgravescs I vaguely remember someone at y! labs telling me (more than a decade back) about MR always doing a sort as part of its shuffle to avoid a variant of this problem by design.
Essentially it boils down to Imran's suggestion even for arbitrary byte writable's [1], [2] ...
[1] https://hadoop.apache.org/docs/r0.23.11/api/src-html/org/apache/hadoop/io/BytesWritable.html
[2] https://hadoop.apache.org/docs/r0.23.11/api/src-html/org/apache/hadoop/io/WritableComparator.html#line.154

@squito @tgravescs I am probably missing something about why hash partitioner helps, can you please clarify ?
IIRC the partitioner for CoalescedRDD when shuffle is enabled is HashPartitioner ... the issue is the distributePartition before the shuffle which is order sensitive but is not deterministic since its input is not deterministic if it is derived from one or more shuffle output's.
Btw, when shuffle = false, it does not suffer from the problem - mentally I had assumed that had an issue too - on a recheck now, I find it interesting that it does not (I never used that, so had never checked in detail !)

Thanks @cloud-fan your summary above is super useful, and I think it's clear enough.

So when we see fetch failure and rerun map tasks, we should track which reducers have its shuffle blocks being rewritten, and rerun them.

IIUC, patterns like rdd.map(...).groupBy() shall always be under risk if we can generate non-determine output in map() right?

Simply inserting a sort before shuffle doesn't help. The fix for dataframe is adding a sort before round-robin, to make it deterministic. If we add the sort after round-robin and before shuffle, the problem still exists.

Does this means, if we can generate non-determine output, then we can still loss some data even add a local sort before shuffle, because the reduce tasks may have already finished (or even have committed)?

be more conservative when handling fetch failure and rerun more reduce tasks. We can provide an internal API to tag a RDD as deterministic (very common in Spark SQL) and then we can safely be optimistic when handling fetch failure.

This is somehow like what I proposed yesterday, one issue we can't resolve is that some ResultTasks may have committed, in that case it seems the best effort we can make is just fail the job.

@squito @tgravescs I am probably missing something about why hash partitioner helps, can you please clarify ?
IIRC the partitioner for CoalescedRDD when shuffle is enabled is HashPartitioner ... the issue is the distributePartition before the shuffle which is order sensitive but is not deterministic since its input is not deterministic if it is derived from one or more shuffle output's.

@mridulm sorry I wasn't real clear, I guess on the RDD side its not called RoundRobinPartitioner (like it is on dataframe side), but the distributePartition is essentially doing that would need to change to just use a normal hash or something that is deterministic.  Basically any operation that does the shuffle has to have deterministic output for choosing which reducers it goes to. The idea at least is what the first PR for this jira was: https://github.com/apache/spark/pull/20414 (disclaimer, I haven't looked at that in detail).  I'll spend more time going through the code to see all the specifics.  But like we've discussed the downside is it will not be evenly distributed.  So the question is if we would want that?

After more thoughts, the problem can be generalized as

RDD#mapPartitions and its friends can take arbitrary user functions, which may produce random result
The shuffle map tasks will consume the inputs and distribute them to different reducers.
When a reduce task throws FetchFailed, Spark scheduler will rerun the map tasks to rewrite the missing shuffle blocks and retry the reduce task. The already finished reduce tasks will not be rerun.

Thanks @cloud-fan  for the write up. This is exactly why I brought up HashPartitioner (ie stop using round robin whether in partitioner or distributePartition function) and exactly why Pig stopped using it for its Union operation.  Its not just us internally doing it, the user code could do anything that the output is not in the same order on rerun of the Map Task.
Like you said, the sort has to be done before the round robin and it has to be done on the entire record (not just on key for instance if you had key,value), and I do see this as being a possibly very expensive operation.  But if for instance we can't change repartition to not be evenly distributed, it seems like one of the only options.  I haven't looked at the details about inserting it here either so I need to do that to understand how complicated it would be.
If we can't come up with another solution, I would actually be ok with failing short term, its better then corruption.  Or perhaps we can actually allow user to choose the behavior, have a config for it would fail, one config for they don't care because they know they checkpointed or something, and one for doing the sort.

I think the correct fix is: be more conservative when handling fetch failure and rerun more reduce > tasks. We can provide an internal API to tag a RDD as deterministic (very common in Spark SQL) > and then we can safely be optimistic when handling fetch failure.

Like @jiangxb1987 said and we discussed above I don't think this will work when you have result tasks that could have committed output.  You can't undo a task commit.
Now we could do a combination of things where as long as we aren't a ResultTask we fail all reducers and maps to rerun. If we are running ResultTask you either fail entire job or fall back to the expensive Sort.
Just to kind of summarize, the only solutions that I've thought of or mentioned by others:

don't use round robin (which for repartition could break the api since it wouldn't be evenly distributed)
Sort all records before doing the round robin assignment ( has to be deterministic) - probably very expensive and need to investigate how hard code would be
Fail all reducers and rerun more if the we haven't started any ResultTasks, if ResultTask decide another solution
Just fail entire job on this type of failure

@cloud-fan I think we have to be clear on the boundaries of the solution we can provide in spark.

RDD#mapPartitions and its friends can take arbitrary user functions, which may produce random result

As stated above, this is something we do not support in spark.
Spark, MR, etc assume that computation is idempotent - we do not support non determinism in computation : but computation could be sensitive to input order. For a given input partition (iterator of tuples), the closure must always generate the same output partition (iterator of tuples).
Which is why 'randomness' (or rather pseudo-randomness) is seeded based using invariants like partition id which result in same output partition on task re-execution.
The problem we have here is : even if user code satisfies this constraint, due to non determinism in input order, the output changes when closure is order sensitive.
Given this, analyzing the statement below :

Step 3 is problematic: assuming we have 5 map tasks and 5 reduce tasks, and the input data is random. Let's say reduce task 1,2,3,4 are finished, reduce task 5 failed with FetchFailed, and Spark rerun map task 3 and 4. Map task 3 and 4 reprocess the random data and create diffrent shuffle blocks for reduce task 3, 4, 5. So not only reduce task 5 needs to rerun, reduce task 3, 4, 5 all need to rerun, because their input data changed.

Here - map task 3 and 4 will always produce the same output partition for supported closures - if the input partition remains same.
When reading off checkpoint's, immutable hdfs files, etc - this invariant is satisfied.
With @squito's suggestion implemented, this invariant will be satisfied for shuffle input as well.
With deterministic input partition - we can see that output of map task 3 and 4 will always be the same - and reduce task input's for 3/4/5 will be the same. So only reduce task 5 will need to be rerun and none of the other input's will change.

I guess on the RDD side its not called RoundRobinPartitioner
Thanks for clarifying @tgravescs ! I was looking at RangePartitioner and variants and was wondering what I was missing - did not make the obvious connection with sql :-)

If we can't come up with another solution, I would actually be ok with failing short term, its better then corruption

If I understand correctly, the proposal is

In ShuffledRDD, add a flag orderSensitiveReducer (?) - to track specific patterns identified which is order sensitive (repartition with shuffle = true, zip, etc).
If a task is getting re-executed as part of stage re-execution, if the flag is true, fail job.

Task re-execution as part of same stage, speculative execution, etc should not be an issue - since only one task completes.
ResultStage should not be affected.
I am unsure about how cache'ing data interacts here - might need some investigation.

This looks like a reasonable stop gap until we fix the issue.
It also allows for users to make progress by inserting a checkpoint before the order sensitive closure to unblock them.

... assume that computation is idempotent - we do not support non determinism in computation

Ah this is a reasonable restriction, we should document it in the RDD classdoc. How about the source (root RDD or shuffle)? The output of reduce task is non-deterministic because Spark fetches multiple shuffle blocks at the same time and it's random which shuffle blocks can finish fetching first. External sorter has the same problem: the output order can change if spilling happens.
Generally I think there are 3 directions:

assume computing functions are idempotent, and also make Spark internal operations idempotent(reducer, external sorter, maybe more). I think this is hard to do, but should be the clearest semantic.
assume computing functions are idempotent and are insensitive to the input data order. Then Spark internal operations can have different output orders. An example is adding sort before round-robin, which makes this computing functions insensitive to the input data order. But I don't think it's reasonable to apply this restriction to all computing functions.
assume computing functions are random. This is not friendly to the scheduler, as it needs to be able to revert a finished task. We need to think about if it's possible to revert a result task.

so I think the assumption is that task results are idempotent but not ordered.  Sorry if that contradictory.   The data itself has to be the same on rerun but the order of things in there doesn't.   That was my general assumption.  I think zip doesn't follow that though when the inputs aren't ordered.  Not sure if there are others spark supports, need to go through the list I guess, unless someone already has?
I think we just need to document these operations and say the results can be inconsistent if not sorted or perhaps give them an option to also sort.  Either that or we have to say we don't support unordered output at all in Spark.    Thoughts on just documenting zip or others with unordered input?
I don't think mapreduce and pig have this issue because they don't internally support an operation like zip, everything is on key/values and joins, groupby on the keys.  User code there could generate it as well but I would claim its the users fault there.

I tried a prototype to fix the handling of fetch failure, seems not that hard: https://github.com/apache/spark/pull/22112

Thanks everyone! I closed this in favor of #22112


SDK Java doesn't reconnect when using SSE

I have a scenario where I use the GrowthBook proxy and use the Java SDK's GBFeaturesRepository class using the SERVER_SENT_EVENTS strategy.
When the application loses connection with the proxy (the proxy may restart or there is a network issue) the GBFeaturesRepository doesn't reconnect to the proxy.

Hello @mvsmasiero!
I suppose connection is stable for 5 minutes and after that connection is being closed.
Is connection lasting, for example, 30min better for you?

It seems like the cause why it doesn't reconnect is Proxy off. When Backend is working GBFeaturesRepository successfully reconnects.

Hello @Bohdan-Kim
When the Java application loses connection to GB Proxy, this method on the GBFeatureRepository is called:

When the connectivity issue is fixed, the application only connects to GBProxy after restarting the application.

It seems like we should make changes to the sources in order to handle this.

The pull request was created according on the issue. We expect that calling initialize() method with retryOnFailure = true will lead to execution of this block:

Hello @Bohdan-Kim , I've tested the solution.
With this solution, the Java application reconnects to GB Proxy successfully, but I've found a problem: if a flag is changed before the application reconnects, that change is not reflected.
I tested this change to fix it. What do you think?

if a flag is changed before the application reconnects
How is it possible that the flag can be changed? Calling initialize() method the second time? The connection already established. Why calling the initialize() method again is needed?

I think the chances of this happening are low, but I believe it is important to evaluate this possibility. For example: The GB Proxy may be down and the Product Owner does not know about it and activates some Flag or makes some other change.

Ok


doc: fix broken markdown table

Summary
Fixes broken markdown tables.
Non related issue.
Requirements (place an x in each [ ])

[x] I've read and understood the Contributing Guidelines and have done my best effort to follow them.
[x] I've read and agree to the Code of Conduct.

 Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

Hi @sisisin! Thank you so much for taking the initiative to submit a fix for this. We greatly appreciate it! 🙂


Scale Set Agents Example is Incorrect

Walking through the docs for creating Scale Set Agents I ran into an issue with the following example:
az vmss create \
--name vmssagentspool \
--resource-group vmssagents \
--image UbuntuLTS \
--vm-sku Standard_D2_v3 \
--storage-sku StandardSSD_LRS \
--authentication-type SSH \
--instance-count 2 \
--disable-overprovision \
--upgrade-policy-mode manual \
--single-placement-group false \
--platform-fault-domain-count 1 \
--load-balancer "" \
--ephemeral-os-disk true \
--os-disk-caching readonly

It generates the following exception:
Deployment failed. Correlation ID: ff93c4ec-e9f5-4da9-bda5-e5748e4a9678. {
  "error": {
    "code": "NotSupported",
    "message": "Ephemeral OS disk is not supported for VM size Standard_D2_v2."
  }
}

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: 0b2476ab-d8db-bd41-689c-8d3b7c4a16d8
Version Independent ID: 6d2e5dd0-8678-d850-a1f3-f664a653bac3
Content: Azure virtual machine scale set agents - Azure Pipelines
Content Source: docs/pipelines/agents/scale-set-agents.md
Product: devops
Technology: devops-cicd-agents
GitHub Login: @steved0x
Microsoft Alias: sdanie

@twurm Thank you for letting us know, I will get this updated, Ephemeral OS disks for Azure VM is supported on premium VM sizes, and the VM size in that example does not use one of the supported sizes on this list.
https://docs.microsoft.com/en-us/azure/virtual-machines/ephemeral-os-disks

@steved0x was the doc updated ? it seems that the example still uses a SKU which is incompatible with ephemerable (didn't try)

@lgmorand Sorry I just saw this one as referenced from https://github.com/MicrosoftDocs/azure-devops-docs/issues/9128, I'll go and double check, for this issue, I removed the ephemeral options from the script and added a note explaining they are only supported for certain skus.
https://docs.microsoft.com/en-us/azure/devops/pipelines/agents/scale-set-agents?view=azure-devops#create-the-scale-set
Removed ephemeral disk options:

Note about supported SKUs and link to where to see which SKUs are supported:

I am reviewing the other GH issue now, this script has evolved so I will run it from scratch to ensure that it hasn't gotten into a non-runnable state...


Content scripts for custom protocols

It does not seem currently possible to inject content-scripts into pages loaded via the protocol handler API. I tested the following methods:

content_scripts manifest key: Custom protocol patterns (e.g. dweb://*/*) are not accepted under the matches key. Also using <all_urls> does not trigger for custom protocols.
browser.tabs.executeScript for tab loading a custom protocol url. This gives me the error: "Missing host permission for the tab".

My use-case for content-scripts on these pages is to inject APIs into the pages. The content-script is required to create a communication channel between the page and the extension background. An alternative to using content-scripts would be a method to add APIs to a protocol, as part of browser.protocol.

Thanks for reporting. It seems that WebExtensions are supporting specific set of protocols only based on the documentation:

The special value "<all_urls>" matches all URLs under any of the supported schemes: that is "http", "https", "ws", "wss", "ftp" and "file".

Presumably list of supported protocols is there to avoid loading content scripts into things like about:, chrome:, resource:. So we'd need to discuss this with the Web Extension team to see what are our options there.

An alternative to using content-scripts would be a method to add APIs to a protocol, as part of browser.protocol.

I think this is reasonable. Still it's likely will need to use same content script infrastructure and to be honest I'm not sure there will be much benefit to this over just content scripts.
I would much rather prefer API like:
navigator.permissions.query({ name: something })

Where addon's could subscribe to requests on something and optionally return implementation. I'm trying to discuss this but it's also unlikely to make it anytime soon.
I think the best bet right now would be to define a REST API to the extension background with a protocol handler itself. And then use fetch or XMLHttpRequest to interact with it.

Filled a bug in bugzilla for this https://bugzilla.mozilla.org/show_bug.cgi?id=1572187 as this can't be addressed here.


Track error stats in the main socket event

This will allow us to determine what kinds of socket errors
are causing problems for users.

Thanks for all the feedback here.  It seems like this is not very appealing, and might not be the best way to go.  I think #142 is probably a better solution.  PTAL.


Preview fails to load

Describe the bug
After most new updates (I enter a chatt into prompt box and bolt modifies code), the preview box shows only a blank white canvas. I refresh, log out, log in, etc. but no change
Link to the Bolt URL that caused the error
https://bolt.new/~/stackblitz-starters-6f2b4l
Steps to reproduce

type instruction into prompt field
hit blue arrow to send command.
wait for bolt to implement changes
Now error occurs

Expected behavior
show visual preview of code output
Screen Recording / Screenshot
No response
Platform
Browser name = Chrome
Full version = <IP_ADDRESS>
Major version = 129
navigator.appName = Netscape
navigator.userAgent = Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/<IP_ADDRESS> Safari/537.36 Edg/<IP_ADDRESS>
performance.memory = {
  "totalJSHeapSize": 44653185,
  "usedJSHeapSize": 43603609,
  "jsHeapSizeLimit":<PHONE_NUMBER>
}
Username = matthewbrunken
Chat ID = 657d3a539448

Additional context
No response

Hi @chatwellai,
Appreciate the feedback here. We are tracking this issue: #1791. You can follow there for updates. If you continue to have this problem, posting the requested details on that issue page would be helpful. Thanks!


Carousel don't recognize the data-interval attribute

Implement the carousel example from the docs: https://demos.creative-tim.com/material-kit/docs/2.0/components/carousel.html
Add the data-interval="10000" attribute to the carousel tag.

What is expected:
Slide interval must change.
What happen instead:
Slide interval not changed and remain default. Even setting data-interval="false" has no effect.

Hi, @speller! Where did you add this attribute on which div from the carousel?
Regards, Dragos

@dragosct10
Here is my carousel code:

    Previous

    Next

Hi! @speller! Sorry for the late response. The data-interval it's not working because already in the project in assets/js/material-kit.js if you will search for carousel you will see that interval is set on 3000, please go there and put the interval or remove the interval from the material-kit.js file and put the attribute on the div .carousel

@dragosct10
Your workaround works, thanks. But what do you plan to fix this issue and avoid hard-coded interval values?

Hi, @speller! Yes, we will fix this in the next update of course, because we want to avoid situations like this.
Regards,
Dragos

Hello @speller,
Thank you for your interest in working with our products, this issue is solved in the latest version (v2.0.4).
All the best,
Stefan


Consider eliminating SFixedBase and UFixedBase

At some point these solved a particular problem, but it seems that is no longer the case.
Perhaps it is time to remove them?
As they are part of the 'details' namespace, they can be removed in a backwards compatible way because nobody should be using them in their code.

This would also fix #40.


PHOENIX-6418 : Bump default HBase version on 2.4 profile to 2.4.2

:broken_heart: -1 overall

Vote
Subsystem
Runtime
Comment

+0 :ok:
reexec
4m 13s
Docker mode activated.

_ Prechecks _

+1 :green_heart:
dupname
0m  0s
No case conflicting files found.

+1 :green_heart:
@author
0m  0s
The patch does not contain any @author tags.

-1 :x:
test4tests
0m  0s
The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.

_ master Compile Tests _

+1 :green_heart:
mvninstall
15m 56s
master passed

+0
hbaserecompile
25m  2s
HBase recompiled.

+1 :green_heart:
compile
1m 31s
master passed

+1 :green_heart:
javadoc
1m 21s
master passed

_ Patch Compile Tests _

+1 :green_heart:
mvninstall
8m 27s
the patch passed

+0
hbaserecompile
43m 59s
HBase recompiled.

+1 :green_heart:
compile
1m 30s
the patch passed

+1 :green_heart:
javac
1m 30s
the patch passed

+1 :green_heart:
whitespace
0m  0s
The patch has no whitespace issues.

+1 :green_heart:
xml
0m  1s
The patch has no ill-formed XML file.

+1 :green_heart:
javadoc
1m 20s
the patch passed

_ Other Tests _

-1 :x:
unit
119m 32s
root in the patch failed.

-1 :x:
asflicense
0m 35s
The patch generated 613 ASF License warnings.

171m 53s

Subsystem
Report/Notes

Docker
ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1169/1/artifact/yetus-general-check/output/Dockerfile

GITHUB PR
https://github.com/apache/phoenix/pull/1169

Optional Tests
dupname asflicense javac javadoc unit xml compile

uname
Linux 437bf748bd6b 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

Build tool
maven

Personality
dev/phoenix-personality.sh

git revision
master / ebe46e1

Default Java
Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08

unit
https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1169/1/artifact/yetus-general-check/output/patch-unit-root.txt

Test Results
https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1169/1/testReport/

asflicense
https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1169/1/artifact/yetus-general-check/output/patch-asflicense-problems.txt

Max. process+thread count
14236 (vs. ulimit of 30000)

modules
C: . U: .

Console output
https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1169/1/console

versions
git=2.7.4 maven=3.3.9

Powered by
Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.


[SPARK-37463][SQL] Read/Write Timestamp ntz to Orc uses UTC timestamp

What changes were proposed in this pull request?
This PR used to fix the issue
https://github.com/apache/spark/pull/33588#issuecomment-978719988
The root cause is Orc write/read timestamp with local timezone in default. The local timezone will be changed.
If the Orc writer write timestamp with local timezone(e.g. America/Los_Angeles), when the Orc reader reading the timestamp with local timezone(e.g. Europe/Amsterdam), the value of timestamp will be different.
If we let the Orc writer write timestamp with UTC timezone, when the Orc reader reading the timestamp with  UTC timezone too, the value of timestamp will be correct.
This PR let Orc write/read Timestamp with UTC timezone by call useUTCTimestamp(true) for readers or writers.
The related Orc source:
https://github.com/apache/orc/blob/3f1e57cf1cebe58027c1bd48c09eef4e9717a9e3/java/core/src/java/org/apache/orc/impl/WriterImpl.java#L525
https://github.com/apache/orc/blob/1f68ac0c7f2ae804b374500dcf1b4d7abe30ffeb/java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java#L1184
Another problem is Spark 3.3 or newer read the Orc file written by Spark 3.2 or prior. Because the older Spark write timestamp with local timezone, no need to read them with UTC timezone. Otherwise, an incorrect value of timestamp occurs.
Why are the changes needed?
Fix the bug for Orc timestamp.
Does this PR introduce any user-facing change?
Orc timestamp ntz is a new feature not release yet.
How was this patch tested?
New tests.

Because my mistake rebase not correctly, I create this PR to replace https://github.com/apache/spark/pull/34712

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50289/

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50289/

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

Test build #145814 has finished for PR 34769 at commit 5347110.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

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

https://github.com/apache/spark/pull/34984 replaces this PR.


Inheritance between a Struct and an Enumeration

A struct is created (by default) under the StructType namespace/folder and an Enumeration is created (by default) under the EnumType namespace/folder.
When a Struct inherits from an Enumeration, the namespace should be fully indicated otherwise it fails.
Currently, the extends statement is:
class AgeCategory extends List_OfferAgeCategory

It should be:
class AgeCategory extends \EnumType\List_OfferAgeCategory

fixed in 2.6.0 and 1.11.0


DOC/API: limit argument to interpolate is somewhat non-intuitive

The current documentation for the limit parameter looks like this:
limit : int, default None.
    Maximum number of consecutive NaNs to fill.

Several features were not immediately obvious to me and could probably be resolved by slightly better docstring or API design:

What happens where the max number of consecutive NaNs is reached? pandas simply stops replacing values, e.g., if there is a gap of 3 values and limit=2, pandas replaces the first 2 values. The alternative would be to entirely ignore gaps of more than 2 values. I'm not saying this would be a better alternative, but we should clarify this.

The limit refers to forward filling only, even though interpolation is not inherently directional. It would be nice if there was some way to trigger a limit for back filling at the same time, e.g., with forward_limit and backward_limit arguments:
>>> pd.Series([1, np.nan, np.nan, np.nan, 5]).interpolate(forward_limit=1, backward_limit=1)
0     1
1     2
2   NaN
3     4
4     5
dtype: float64

Thoughts?

@shoyer this is very well defined when reindexing (e.g. for your first point, you simply don't fill forward/backward anymore). So should be the same for interpolation (and btw, you can require that a method be provided, e.g. forward/backward  when using limit, no need to add another kw. This should only be uni-directional, way to complicated otherwise.

@jreback  I agree this is well defined for reindex. But interpolate makes no reference to backward or forward filling, regardless of the method (e.g., consider method='linear', the default). Hence the confusion.

ahh that's right
but I think the direction of interpolation is by definition forward always right? (as these are mostly scipy routines) ?

Take a look at the scipy routines -- they really make no reference to forwards or backwards. They just interpolate between points. As you can see here, we handle the limit argument in pandas itself: https://github.com/pydata/pandas/blob/e2b014ce6791dc1907b9ed4dd3cd23cacea28af3/pandas/core/common.py#L1538

no what I mean is interpolation is by definition forward ATM
as their is no way to specify it
I mean u can just flip the array of backward and flip after done
can think about a direction kw

@shoyer correct, we do all the limit handling.

The alternative would be to entirely ignore gaps of more than 2 values

I think I remember a SO post asking how to do this actually...

The limit refers to forward filling only, even though interpolation is not inherently directional

I guess it depends on the context. You might not want to fill backward for some time series. But that's up to the user to decide, so having the option would be nice.
As far as implementation goes, it shouldn't be too difficult, just rework the limit logic a bit. I wrote that originally, but I'm not sure I'll have time to get to this soon :disappointed:

I just tried adding a little bit of code to address this issue!
I've never contributed to Pandas before so I'm not sure what the procedure usually is. I'd be happy if there was some discussion to make sure this change makes sense. What should I do next?

Contribution guidelines are here
And there's some more information in the wiki

Ok, thanks!
I suppose the discussion about expected behavior, etc. will take place on the PR.

The alternative would be to entirely ignore gaps of more than 2 values

I think I remember a SO post asking how to do this actually...

For posterity, here is the SO post that discusses this:
https://stackoverflow.com/questions/30533021/interpolate-or-extrapolate-only-small-gaps-in-pandas-dataframe


Update NodeJS to v16.10.0

Update NodeJS to v16.10.0 which is the last v16 release bundled with npm 7.

Good idea to update NodeJS for next release ... but isnt v16.13.1 the lastest LTS version? Any particular reason why v16.10.0?

@rootzoll we recently upgraded mempool to angular 13 and there were some dependency issues with node 16.13+npm 8 so we pinned to the latest v16 which came with npm 7.
I think ideally we should be using a node version manager like nvm so different apps don’t have to rely on the system wide node installation

Yeah nvm could be an idea for the future, but that would be some deeper changes. For the v1.7.2 I will then give v16.10.0 & npm7 a shot and see how the other apps work on it. Mergeing into dev for final testing.


Add mustBeCongruentSizes(a, b)

There's mustBeSameSize(a, b) that checks whether two inputs are exactly the same size. Would be nice to have an alternate of this that checks whether the two inputs are the same size or some of them are scalars or they're all "broadcast-compatible" – that is, it checks that sizes along all nonscalar dimensions are the same, but allows them to be scalar along any dimension.
I'm not sure what the right name for this is. Maybe one of these?

mustBeConformant()
mustBeConformantSizes()
mustBeBroadcastable()
mustBeCompatibleSizes()
mustBeCongruentSizes()

Done in https://github.com/apjanke/matlab-validoozy/commit/4dedd6e6e83b9cdc925768f1ea2e3db50c20526f.


Bump typescript to 4.1

Change-type: patch
Signed-off-by: Thodoris Greasidis<EMAIL_ADDRESS>
Contributor checklist

[ ] Includes tests
[ ] Includes typings
[ ] Includes updated documentation
[ ] Includes updated build output

@balena-ci retest

@balena-ci I self-certify!


Update PBAR documentation after conversion

Adjust security best practices for pbar instead of request handlers
Remove release/conversion announcements, they're in the past now
Fix typo for top level location ' -> /

I want to leave the request-handlers-to-pbar.md for a bit. It's not
referenced from other pages now, but it's referenced from the conversion
announcement email that customers got. We can remove the page after all
dust settled down, e.g. in Oct 2017. I'll send a separate PR as a
reminder for that.

Hans, please sign the CLA.


Fixes #145 changes canonicalization method for metadata xml signature…

Coverage remained the same at 95.267% when pulling 7a715e0168cb2a25250cf96793c24ecc32b3fa96 on dave-handy:bugfix/145 into 1ada1e760c8f037e224a9dace7eeebb347541b3b on onelogin:master.


Make initialisms all caps

See https://github.com/golang/go/wiki/CodeReviewComments#initialisms
Following from conversation here: https://github.com/coreos/rkt/pull/1945#discussion_r49264529https://github.com/coreos/rkt/pull/1945#discussion_r49264529

LGTM

LGTM, this is going to break consumers but let's rip that bandaid off


Added line 'Since Gatsby is Built on React'

Outlining more reason to consider giving Gatsby a trial.....

[x] I have read freeCodeCamp's contribution guidelines.
[x] My pull request has a descriptive title (not a vague title like Update index.md)
[x] My pull request targets the master branch of freeCodeCamp.
[x] None of my changes are plagiarized from another source without proper attribution.
[x] My article does not contain shortened URLs or affiliate links.

If your pull request closes a GitHub issue, replace the XXXXX below with the issue number.
Closes #XXXXX

Closed and Reopened this PR to attempt to resolve a specific Travis build failure.

Thank you for your contribution to the page! 👍
We're happy to accept these changes, and look forward to future contributions. 🎉


This library is super awesome!

Thank you creating it.

+1

+1

Ah! You beat me to it.
Also chiming in, and the new docs are awesome.

+1

Indeed! Many thanx, Julian! 👍

+1


"Mixed Mode" routing - Cannot make it work

I have a typical MVC site that I want to have mixed routing on - I have some fixed pages with coded controllers which I want to use (I've created PageTypes in code for them). I want to use the CMS capability to allow users to create new pages in the site, but cannot get the routing to work.
I've turned PassiveMode to False.
in my RouteConfig I've got;
routes.MapRoute(
"Default",                                              // Route name
"{controller}/{action}/{id}",                           // URL with parameters
new { controller = "Home", action = "Index", id = "" }  // Parameter defaults
);
I've tried;
routes.MapRoute(
name: "Default",
url: "{controller}/{action}/{id}",
defaults: new { controller = "Home", action = "Index", id = UrlParameter.Optional },
namespaces: new[] { "xxx.Controllers" }
My problem is that I cannot access any pages created by the user in the CMS but I can access my hard coded pages (which call out to the CMS via Model Properties).
There is something I'm missing here - can anyone help ?

I'm very puzzled about this - when I examine the MVC RouteTable.Routes with PassiveModel set to False I don't see any routing at all for my Piranha pages - only for the authorisation and manager controllers. I'd expect to see a controller added and removed when I switch between PassiveMode True and False. Perhaps this is a clue to why my CMS pages don't get served up ?

Hi there! If you run passive mode no CMS-pages will be served by the routing, in your case you should probably stick to having it as true.
Now the routing in Piranha CMS is very simple, it runs as a HttpModule that intercepts the request before it is passed on to MVC.

The request is checked if it matches any permalinks for pages, posts or media
If it does the request is simply rewritten to the controller/action route for that page/post type
If the request doesn't match any permalink it is passed on to ASP.NET MVC

This is true for everything EXCEPT for the start page. Requests to "~/" will always be served by the start page of the site.
Regards

I see that in the source code - but still; with passiveMode = False I get passed through all the requests for my CMS managed pages into my MVC route, which the fail. I am using IIS Express which forces localhost:98749 (random port number) but I guess your just using the localhost portion, so I've "localhost" in the site hostnames.
In order to try to eliminate the use of "localhost" (which is obviously not ideal) I've tried moving the project to a local IIS and using my machine host name, but I'm getting a flood of SignalR messages from your CMS pages back to the original localhost:98749 location causing cross-site-scripting issues.
Does any of this make any sense to you as a pattern of problems ?

Hi there! Getting similar issue. I really like the concept, manager and overall functionality of Piranha, but struggling with integrating it.
I want to have partially CMS to integrate to MVC application that has some hard coded functionality, allows administrator to edit parts of some pages and completely manage (create sub pages etc of another).
So for sake of testing I went with default MVC template. I have there homecontroller with index, about etc. pages.
If I run application with passiveMode value="false" I'm able to create pages. Custom pages with custom views. But even with  I can't get any of standart MVC controllers. So for instance I don't have /Home/About in CMS and I get 404 error instead of routing to controller as it should according to post above.
If I turn passiveMode ON, then (as it should be) I have my controllers working as in any MVC application but permalinks not working. As far as I understood from docs If I want to get content from CMS I should inherit from SinglePageController (HomeController  : Piranha.Mvc.SinglePageController) and edit view to display what I need. I've spent quite some time trying to get the answer how to avoid without result and recive [NullReferenceException: Object reference not set to an instance of an object.]
Piranha.Models.Page.GetByPermalink(Guid siteTreeId, String permalink, Boolean draft). It doesn't matter if I have permalink with the same name or not looks like Request["permalink"] = '((System.Web.Mvc.Controller)(this)).Request' is null and this breaks in BaseController class. That is probably different question but for the same issue.
In conclusion. Looks like Me and Phillip are solving the same problem. But getting completely different results. With passiveMode false he gets MVC route and I get CMS always. Plus it is not clear how to get pages working in passivemode.

You are all right!!! I finaliy realized what you meant and YES, there is actually a bug in the current version that was a result from a pull request in one of the more frequent versions. The issue was resolved last year and I pushed it to the master branch:
https://github.com/PiranhaCMS/Piranha/commit/e4ee5c581af636f70c1b5309226bd823e1c89b1a
However this doesn't help you much if you install the package from NuGet. Either you get the latest version here from GitHub, or I could put an Alpha up for the upcoming verison. You'll get a bunch a good stuff (check it out at https://github.com/PiranhaCMS/Piranha/milestones/Version 2.2.6) but the new Blocks feature isn't 100% finished.

Thank you for your answer. That is a good news! Having Aplha will be great. If not I'll get master branch and report back soon once I rebuild project manually without nuget it in my scenario.

I finally had time to check it further. I used master branch and experimented using template site (as I had some issues with clean MVC site probably because I missed some step in moving necessary files and setting all configurations manually)
I have my **problem solved only partially. **
First of all I can confirm that having CMS (with passivmode=false) let me use my controllers side by side. If no permalink with certain address I can have my controller if there is permalink I get what CMS let me see. That is great!
The problem I still have (or don't understand logic) is when I extend controller inheriting from SinglePageController or if I set passivemode=true and try to go by permalink adress. In both of this cases I get NullReffeence error form GetByPermalink method.
It doesn't even matter what I put into Action methods - that code won't be reached as the issue will be in controller inicialisation. To replicate this behaviour I was following https://piranhacms.org/docs/models/extending-models instructions the moment I inherit from singlepagecontroler all breaks up.
Is it me missing something or it is a bug? If it is not a bug what is the right way to run custom controller that has it's logic and model to combine it's content with content availible for edit in CMS?

Hi there! The singlepage & singlepost controllers can only be used for pages & posts that are routed through the CMS and rewritten internally. Direct accesses to the controller will fail as the base classes tries to retrieve the current permalink from the internal request!

Thank you for little clarification.
Ok, I tried having say testController and page created in CMS with permalink test/index. I suppose that with controller and permalink having the same route controller should have been called. What is the right way to do achive this goal?
So basicly two questions:

What is the way to have cotroller with some custom logic and CMS render page with content from both (as was in example - list products form database and have an ability for content editor to put some contenent on part of this page)?
In passive mode. What is the way to render page from CMS?

To achieve (1) I just add the pages to the CMS system, and in Razor just use the Piranha HTML Helpers to pull static data from the CMS. I get to choose what content I delegate to the CMS that way. Of course, I'm still stuck in being unable to render a "pure" CMS page that doesn't have any MVC code behind it.

Hi, thank you Phillip. Sounds like a nice solution.
I've already tried to find helper. I found two (UI and Site). There are a few very useful such as creating links or render head section. But I can't see a method there that let me, for instance render html section? I even browsed through the sources and didn't find such method. Have not found solution in wiki either.

I can explain how it works tomorrow, I think you're just approaching it the wrong way!
Regards

Looking forward to it if there is the better way.

Hi Håkan, so any chance for finding time to put some words for a better solution...

Hi,
Did you ever release a fix for Mixed Mode as a pre-release on NuGet ? I can't find it. Sad to say but I'm pretty much going to have to drop this unless I can get it working quite soon.
Have you got any tracing I can look at in terms of the Http intercepts and decisions made for the routing decisions ? Can I hook any events to try and find out why I just cant open my MVC pages after I install and configure Piranha MVC ? I've tried under IIS Express and full IIS, using localhost and named host and still can't get it working reliably (or at all having removed and reinstalled Piranha). Tought I'd use Glimpse to try and see whats going on, but its killed Glimpse as well :-(
I'll give it one more try by downloading the source code and integrating at that level - at least I'll be able to debug it to see where my page calls are being killed.

I've been away on vacation getting some well deserved time off, but I've uploaded the current beta packages of Piranha CMS to NuGet. You can update your existing packages by including pre-releases in the package manager.

That is great news! Hope vacations was great as well. Could you please write here or somewhere in docs what is best practice to achieve questions 8 posts above?

And here is the simple explanation on how the routing & CMS works.
Page & Post Types
Almost all CMS's out the are based on templates in one way or another. In Piranha this is called Page Types and Post Types. A type defines the type of content a page of this template can have. For example you could have the page types.

One Column Page
Two Column Page

The pages would have one and two html regions. When requests are made for pages created in the CMS of these types the request then has to be rewritten to a route that will handle the data and then render it by the correct view. All controllers handling requests rewritten by the CMS (this is i active mode) should inherit from either SinglePageController or SinglePostController
Adding data from outside the CMS
You could for example have a page displaying some kind of data not controller by the CMS, let say products. The correct way of turning this into a Page Type would be to first create a page type called Product Page. This page type should contain the content regions needed, but could also contain for example a product category or something else used to filter the products from outside the CMS.
When setting up the model for the ProductPage view you could either inherit from PageModel/PostModel, or do a custom model with a property for the CMS data, i.e:
public class ProductPageModel1 : PageModel {
  public IList<Products> { get; set; }
}

public class ProductPageModel2 {
  public IList<Products> { get; set; }
  public PageModel CMSData { get; set; }
}

Which solution works best is dependent on your application and the amount of code you had before installing Piranha.
Handling a lot of application pages
In many cases with existing application you already have a vast amount of controllers doing stuff. You might want to add these into the CMS to let content managers control the structure of the site, but adding a Page Type for each different page would not be feasible. A simple solution would then be to create a single Page Type called ApplicationPage. This page type could contain some basic content fields, like a HTML region for adding text.
The trick is to on the Page Type set the option Allow pages to change the route (you can read about this in section 2.5 at http://piranhacms.org/docs/pages/page-types). This way you could reuse this Page Type for all of you application pages, setting the route of the page on the page instance and not cluttering the CMS admin with useless Page Types.
Passive mode
Passive mode should ONLY be used when you have a finished, already completed site and you realize afterwards that you need to allow managers to update content. By enabling passive more ALL ROUTING for pages & posts are disabled and only your own controllers will respond to request.
You can then add Page Types as described above and build a site structure containing the content needed to be administrated from the CMS, but you will then need to load the correct PageModel manually in each of your controller/action and add it to your models.
I hope this helps to clarify the setup a bit.
Regards

Hi tidyui - I get the theory - I just don't experience it working in practise.
Heres my experience of working with an existing MVC application; this application is working under IIS Express.
Installed PiranhaCMS.MVC 2.2.6-beta, and altered the RouteHandler as documented. Altered web.config to remove duplicate Authentication tag.
Tried logging in to /manager using known good credentials. Manager page does not log in but just refreshes. Entered wrong credentials - manage page correctly reports "incorrect credentials". Assume inability to login is a beta issue.
Tried opening my Home page (no authentication required - anon access and no [Authorize] directive). Got 404. All my pages report 404.
Switched passiveMode = true (in desperation). My pages now render, but of course, since there is no server side URL rewrite then I would not be able to include CMS pages alongside my hard coded pages.
What I desperately need is for you to have a logging capability so I can actually trace the re-write decisions the platform is actually making, otherwise were just going to be stuck in this loop of you telling me "it works" and me saying "no it doesn't". Don't get me wrong, its a nice platform - but I just can't make it work as advertised currently. It seems the Achilles heel of the underlying design is the auto-magic url rewriting (why not use MVC routes on your MVC version - its what they were designed for :-) ) and that its not debuggable or diagnosable.

So, I've created a new repo here where I've added an MVC example for Mixed Mode. In my example I've assumed that you've added the package piranha.mvc from NuGet @PhillipHamlyn.
https://github.com/PiranhaCMS/Examples
I followed the guidlines from Adding Piranha CMS to an existing application at http://piranhacms.org/docs/introduction/getting-started except for letting passiveMode be false.
For me the setup works perfectly exept for the fact that CMS generated pages doesn't work. The reason for this is that the template package piranhacmsmvc bundles two controllers and views for rendering CMS content. These are of course not present in the lib setup of piranha.mvc.
In my example repo I've added the default controllers & views for pages & posts.

Thanks for making the repo - I've downloaded it and linked against my CMS DB and yet :-) it works.
I found the issue with logging in to the manager and perhaps a clue to my woes. If I set the web application settings to SSL Enabled and then access the site via Https then no manager or other pages are rendered. My sites are always configured as Https. I never thought to turn that off.
I reconfigured my site to use Http and now I can now load the management pages (horay !). All my controllers are marked with [Authorize][RequireHttps] which may be causing the issue. Have you been able to make your repo work over Https ?

Yes I could definitely look into https, I wasn't aware there was an issue with it and no one's reported it :)
I didn't quite follow your first paragraph though, did the example repo work for you?

Because if the test repo works I'm closing this issue and marking it for release with 2.2.6 and then I suggest we open a new issue for https!

Yes - Ok - the repo worked for me Http only. Thanks for your help. I will temporarily strip out my [Authorize][RequireHttps] and see if I get further.


fix Fix erase MemTrackerLimiter from tracker pool

Proposed changes
wrong usage of weak ptr in MemTrackerLimiter tracker pool
Further comments
If this is a relatively large or complex change, kick off the discussion at<EMAIL_ADDRESS>by explaining why you chose the solution you did and what alternatives you considered, etc...

run buildall


Changing wallet on Phantom chrome extension doesn't update publicKey anymore

Describe the bug
Changing wallet when using Phantom chrome extension is not triggered in react (I am pretty sure it used to work on previous versions)
For instance the following minimum example in React works when connecting a wallet or disconnecting, but not when changing one wallet by another:
import {useWallet } from "@solana/wallet-adapter-react";

export default function SomeReactComponent() {
  const { publicKey } = useWallet();

  useEffect(() => {
    if (publicKey) {
        console.log("Wallet changed!", publicKey.toBase58());
   } else {
       console.log ("Wallet disconnected");
   }
  }, [publicKey]);

  return <></>
}

To Reproduce
Steps to reproduce the behavior:

Using the minimum example provided within a Connection provider.
Connect one wallet using Phantom chrome extension.
"Wallet changed!"will be displayed in the console.
Go to Phantom extension and choose a different wallet.
"Wallet changed!"should be displayed again showing the new wallet address, but doesn't work anymore.

Expected behavior
"Wallet changed!" should be logged whenever the wallet is changed.
Desktop:

OS: OS X
Browser: Chrome
Version:

    "@solana-mobile/wallet-adapter-mobile": "^2.0.1",
    "@solana/wallet-adapter-base": "^0.9.23",
    "@solana/wallet-adapter-react": "^0.15.34",
    "@solana/wallet-adapter-react-ui": "^0.9.33",
    "@solana/wallet-adapter-wallets": "^0.19.20",

The issue can be replicated even in your demo:

Connect one wallet using Phantom chrome extension
Click on sign a message (you will see that the wallet address is the correct one)
Go to Phantom extension and change the wallet.
Click again on sign a message, the wallet address is the old one.

This probably isn't an issue with Wallet Adapter, it's almost certainly a bug in new version of Phantom. Nothing relevant in the Phantom adapter has changed, and the demo/example is not using the Phantom adapter anyway.
Closing here but tagging @bfriel for help.

Thanks for the report @AnderUstarroz . This is indeed a Phantom-specific bug which we believe is only occurring on Solana-only Mode. We have a separate thread tracking this here.


phase_name is not being captured in Sysmon EventID 5

I have followed your guides, however phase_name is not being captured in Sentinel:

However the parser was installed successfully:

And I can clearly see the phase_name in the sysmon event on the server:

So I'm a little lost. Also, when querying the log, the column names do not seem to correspond with the data they contain, which suggests perhaps an issue with the parser (?):

Originally posted by @srthomson in https://github.com/BlueTeamLabs/sentinel-attack/issues/15#issuecomment-532157466

Unfortunately it's not being captured in any if the EventID's, the issue is not isolated to just EventID 5.

Hi @srthomson , i'm struggling to replicate the behaviour you are describing. I have run a couple of tests on a Win10 machine using the latest version of Sysmon (v10.4), the parser contaning latest fixes and the sysmonconfig.xml from our repository. In all cases where sysmon captures RuleName information this is picked up by Sentinel and phase_name is correctly parsed by the Sysmon-OSSEM parser. For reference I attach a screenshot of a test command i ran:

And the related log picked up and correctly parsed within Sentinel:

Maybe i'm missing something; in order to better help i suggest you reinstall the latest Sysmon-OSSEM parser in Sentinel and re-run some tests. Keep in mind that it might take a couple minutes for the parser to become effective within Sentinel. Once you've done that please let me know the exact steps you performed, with an accurate description of the environment used, and i'll do my best to replicate.
On a sidenote i've noticed errors in the parsing of Event ID 1; i've opened issue #18 to address this bug later on.

Re-installing the latest parser fixed the issue. Thank you.