Tool crashes after computing descriptors

I am trying to use the tool to automatically align two point clouds that have a lot of overlap. Unfortunately, I cannot release the point clouds for privacy reasons, but I can perform tests that may help to track down the source of the bug.
The command output looks like this:
Executing in parallel
Loaded point cloud with 240283 points:
Scan 3.ply
Loaded point cloud with 849421 points:
Scan 2.ply
Computed source cloud normals (240283)
Computed target cloud normals (849421)
Subsampled source cloud from 240283 -> 204804
Subsampled target cloud from 849421 -> 722911
Computed 204804 descriptors on source cloud keypoints
Segmentation fault (core dumped)

I ran the tool in GDB and got the following backtrace:
(gdb) bt
#0  __GI___libc_free (mem=0x9600d4) at malloc.c:3103
#1  0x00000000080698cb in boost::detail::sp_counted_impl_p<pcl::PointCloud<pcl::ReferenceFrame>>::dispose() ()
#2  0x000000000803c19a in boost::detail::sp_counted_base::release() ()
#3  0x000000000806cb67 in pcl::SHOTEstimationBase<pcl::PointXYZ, pcl::Normal, pcl::SHOT352, pcl::ReferenceFrame>::~SHOTEstimationBase() ()
#4  0x00000000080613d5 in Registrator::computeDescriptors() [clone ._omp_fn.4] ()
#5  0x00007ffff6c96c2e in ?? () from /usr/lib/x86_64-linux-gnu/libgomp.so.1
#6  0x00007ffffefd76db in start_thread (arg=0x7fffde150700) at pthread_create.c:463
#7  0x00007ffff678188f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb)

I am running this on Ubuntu LTS 18.04 on Windows 10. I am running VcXsrv in case the tool's --gui mode is trying to connect to an X server. The nature of the error makes me suspect that this is a logic error in the code, and not something specific to what platform it is running on.
What is my next step?

I can add that since this crash is occurring during the deletion of an object, I decided to see what would happen if I worked around it. I went to the Registrator class and made its allocation of pcl::SHOTEstimationBase<pcl::PointXYZ, pcl::Normal, pcl::SHOT352, pcl::ReferenceFrame> in computeDescriptors a heap allocation instead of a stack allocation. I allowed it to leak intentionally so that when computeDescriptors returns, it doesn't try to free it, avoiding the crash. I didn't have high hopes for this, because the crash suggests that something within the parent data structure is corrupt, and that corruption will probably affect things that follow as well. But, I fired it up anyway, and I got the following output:
Executing in parallel
Loaded point cloud with 240283 points:
Scan 3.ply
Loaded point cloud with 849421 points:
Scan 2.ply
Computed source cloud normals (240283)
Computed target cloud normals (849421)
Subsampled source cloud from 240283 -> 204804
Subsampled target cloud from 849421 -> 722911
Computed 204804 descriptors on source cloud keypoints
Computed 722911 descriptors on target cloud keypoints
Found 706607 correspondences
Retained 706607 correspondences after sample consensus filtering

At this point, the code then crashes with a SIGSEGV and the following backtrace:
(gdb) bt
#0  0x0000000008076933 in pcl::registration::TransformationEstimationSVD<pcl::PointXYZ, pcl::PointXYZ, float>::estimateRigidTransformation(pcl::ConstCloudIterator<pcl::PointXYZ>&, pcl::ConstCloudIterator<pcl::PointXYZ>&, Eigen::Matrix<float, 4, 4, 0, 4, 4>&) const ()
#1  0x0000000008076ad5 in pcl::registration::TransformationEstimationSVD<pcl::PointXYZ, pcl::PointXYZ, float>::estimateRigidTransformation(pcl::PointCloud<pcl::PointXYZ> const&, pcl::PointCloud<pcl::PointXYZ> const&, std::vector<pcl::Correspondence, Eigen::aligned_allocator<pcl::Correspondence> > const&, Eigen::Matrix<float, 4, 4, 0, 4, 4>&) const ()
#2  0x00000000080638cb in Registrator::computeCorrespondenceBasedTransformation() ()
#3  0x0000000008064461 in Registrator::correspondenceBasedRegistration() ()
#4  0x00000000080646a8 in Registrator::performRegistration(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) ()
#5  0x0000000008035a75 in main ()
(gdb)

Does this get us any further??


Investigate relay memory buildup

It has seen that cache memory increases over time when there is active session. Need to see if we are leaking memory

I think I understand what is happening. The code of quicrq_app.c includes:
    /* If relay or origin, delete cached entries longer than 2 minute */
    quicrq_set_cache_duration(cb_ctx.qr_ctx, 120000000);

That seems quite high. Two minutes of build up means something like 5,000 video frames, probably about 10MB. Multiply by many streams and many clients, that can mean quite a lot of memory. We want a shorter timer, but how much shorter?

@huitema  as chatted, 10s would do for this timer.

Timer was set to 10seconds in PR #112. Issue is reported fixed after trials of new version.


Support HTTP(S) Proxy with Authentification

Tasks:

[ ] Install and set up HTTP(S) proxy with Authentification.
[ ] How does Nuvola Player 3 behave?
[ ] Does glib/gio proxy resolver support HTTP(S) proxy with Authentification?
[ ] Is it possible to use environment variables?

Original report:

Request support  for Proxy with authentification (HTTP/HTTPS ?) or use Network-Manager or /etc/environnement variables.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

Hello @Auze. Is this still an issue in Nuvola 4.x? Thanks.

Closing because there has been no response from the reporter over a month. Feel free to reopen this issue if it still persist.


Can the "data viewer" in VS Code filter data with multiple conditions?

Can the "data viewer" in VS Code filter data with multiple conditions?
For example: ">100" is effective, and "<200" is effective. Is it possible to filter the data of "100-200"?
actual:

The code I used:
import pandas as pd
import seaborn as sns
import matplotlib.pyplot as plt

tips = sns.load_dataset("tips")
print(tips.head())

sns.scatterplot(x = tips['total_bill'], y= tips['tip'])

plt.show()

Related links: vscode-python - Can I use the data viewer's filters to apply 'greater-than' and 'less-than' filters simultaneously?
Thanks a lot!

Data Viewer in VScode to view, sort, and filter the rows of data.
I can use > and <, which is great
I am wonderng if there is a way use something contain: string similar to .str.contains('search_string')

closing as the jupyter variable viewer was deprecated https://github.com/microsoft/vscode-jupyter/issues/15486


feat(1854): Fix cases of local variable deletion in dead store elimination

Fix #400
Fixes the problem described in #400 with a pretty cool mechanism for moving the declaration into appropriate scope. See the updated handled rules for a concise description of the new mechanism.
Note: There are still cases where this may crash and burn if there are multiple dead stores in a row in the same variable. That can cause issues, but I'll leave that for a later PR. We need some mechanism to indicate which elements have been repaired, so we don't attempt to enact repairs on already modified elements.
And yes, I spent way too much time on this, but it was a super fun problem to solve :)

@khaes-kth @fermadeiral I need this merged for the demo tomorrow, so if either of you have time to review anything before then please prioritize this PR.

LGTM.
@slarse, sorry for the delay, last week was crazy and I didn't notice this PR.

That's OK, it's in for the demo so we're all good :)


Not able to add stream data to redis using Nodejs

Hi Team,
I am trying to add data to the redis stream using Nodejs but the data could not be added. I am using below code:
redisClient.sendCommand(XADD ${streamName} * name test, [], function (error: any, res: any) {
console.log(error);
console.log('a', res);
});
I am using redis version 3.1. Can you please help me with this?
Regards,
Chirag Khanna

The Redis Server version is 3.1 or the client version is 3.1? cause Redis streams were introduced in 5.0..

Hi @leibale,
The client version is 3.1 and server version is above 6 and we tried above query as well but it doesn't works as well.
Regards,
Chirag Khanna

@chiragkhanna28 can you please share the error message?

(node:15972) UnhandledPromiseRejectionWarning: ReplyError: ERR unknown command 'X'
at parseError (C:\xampp\htdocs\registration-service\node_modules\redis-parser\lib\parser.js:179:12)
at parseType (C:\xampp\htdocs\registration-service\node_modules\redis-parser\lib\parser.js:302:14)
(Use node --trace-warnings ... to show where the warning was created)
(node:15972) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag --unhandled-rejections=strict (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:15972) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
when doing this
await client.sendCommand('XADD', [streamName, '*', 'name', 'test'], function(err, reply) {
        console.log(err, reply);
    });

receiving same error here.
await client.sendCommand('XADD', [streamName, '*', 'name', 'test'], function(err, reply) {
        console.log(err, reply);
    });

Error:
(node:19769) UnhandledPromiseRejectionWarning: ReplyError: ERR unknown command 'X', with args beginning with: 'A', 'D', 'D',
    at parseError (/Users/danial/code/wss-rate/node_modules/redis-parser/lib/parser.js:179:12)
    at parseType (/Users/danial/code/wss-rate/node_modules/redis-parser/lib/parser.js:302:14)
(Use 'node --trace-warnings ...' to show where the warning was created)
(node:19769) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag '--unhandled-rejections=strict' (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:19769) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

also similar error with
await redisClient.sendCommand('XREAD', ['BLOCK', 1000, 'STREAMS', 'streamName', '$'], function (err: any, reply: any) {
            console.log(err, reply);
        })

Error
(node:19740) UnhandledPromiseRejectionWarning: ReplyError: ERR unknown command 'X', with args beginning with: 'R', 'E', 'A', 'D',
    at parseError (/Users/danial/code/wss-rate/node_modules/redis-parser/lib/parser.js:179:12)
    at parseType (/Users/danial/code/wss-rate/node_modules/redis-parser/lib/parser.js:302:14)
(Use 'node --trace-warnings ...' to show where the warning was created)
(node:19740) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag '--unhandled-rejections=strict' (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:19740) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

in my case the reason of using Raw redis commands in the first place is after i'm adding a stream with "xAdd "
await redisClient.xAdd('streamName', '*', "some:key", "test-value").then(function (id) {
            console.log("id:", id);
        });

# id:<PHONE_NUMBER>428-0
# id:<PHONE_NUMBER>308-0
# id:<PHONE_NUMBER>266-0
# id:<PHONE_NUMBER>637-0
# id:<PHONE_NUMBER>358-0
# id:<PHONE_NUMBER>458-0

i can't get the stream with "xRead" as it's always returning this error
(node:19642) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'length' of undefined
    at encodeCommand (/Users/user/code/project/node_modules/@node-redis/client/dist/lib/commander.js:71:104)
    at encodeCommand.next (<anonymous>)
    at RedisSocket.writeCommand (/Users/user/code/project/node_modules/@node-redis/client/dist/lib/client/socket.js:56:20)
    at Commander._RedisClient_tick (/Users/user/code/project/node_modules/@node-redis/client/dist/lib/client/index.js:421:64)
    at RedisSocket.<anonymous> (/Users/user/code/project/node_modules/@node-redis/client/dist/lib/client/index.js:338:86)
    at RedisSocket.emit (events.js:315:20)
    at RedisSocket._RedisSocket_connect (/Users/user/code/project/node_modules/@node-redis/client/dist/lib/client/socket.js:136:10)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async Commander.connect (/Users/user/code/project/node_modules/@node-redis/client/dist/lib/client/index.js:156:9)
(Use 'node --trace-warnings ...' to show where the warning was created)

can anyone help plz?!

Ok, the right way to use "XADD" and "XREAD" through the sendCommand is
await client.sendCommand(['XADD', 'streamName', '*', 'name', 'test'], function(err, reply) {
        console.log(err, reply);
    });

await redisClient.sendCommand(['XREAD', 'BLOCK', '1000', 'STREAMS', 'streamName', '$'], function (err, reply) {
            console.log(err, reply);
        })

Closing as resolved.


Not possible to disable "Lock app with FaceID"

Whenever I toggle off the switch for faceid locking, the app doesn't save the change persistently.
This means that there is essentially no way to disable the feature, other than clearing NSUserDefaults by deleting the app.
https://github.com/paulgessinger/swift-paperless/assets/63339559/21e256fb-a255-4742-a048-958596a654a7

I can reproduce this, thanks for reporting @SoCuul

Can you check build 83 on TestFlight @SoCuul?

Completely fixed! 🎉


AddressMapper client

Removed the private key from constructor, passing the address, also added a factory static method to create the contract and return AddressMapper  ready to use


Medical Legal Department

@jwittenbach @mcocanougher  Welcome Matthew! You can update us on any HIPPA compliance issues here. Feel free to include links if you want us to see where you're looking; of course we probably won't understand what they mean, so you can also just give us a summary of what we should do moving forward.

Here's the best article I've found so far:
http://www.thesecurityadvocate.com/2014/06/05/compliance-for-developers-of-medical-applications-and-software-under-hipaa-and-other-regulations/
I'm now looking more specifically at whether or not the app would make you a "business associate" under HIPAA which would require additional safety steps to make sure patient information was not disclosed.

@mcocanougher This is a very nice article. Ideally we wouldn't be storing any information on the app itself; we would provide a save button that would allow the information to be stored locally on the user's device. I'm not sure if we are then responsible for how they use it. I would hope not.

@mcocanougher @jwittenbach The HIPAA disclaimer is now here. We can edit it from there.


Add validate_cluster=true

@ldemailly yes, I would like to patch and test, this is why I asked you to rebase that PR

I checked with Envoy and this is what we are supposed to do, this is the correct Envoy behaviour. We should have done this already. Do you have some arguments to support why this is not a good idea?

Shriram just explained it, but there are at least two problems with this
approach:

RDS needs to be blocked on EDS update, not just CDS update which is not
possible
What happens when clusters are removed?

On Wed, Feb 14, 2018 at 5:02 PM Andra Cismaru<EMAIL_ADDRESS>wrote:

I checked with Envoy and this is what we are supposed to do, this is the
correct Envoy behaviour. We should have done this already. Do you have some
arguments to support why this is not a good idea?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365795866, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxpI21W4QIDX7x5fexvCzgnFix6chks5tU4I0gaJpZM4SEx-i
.

see https://github.com/envoyproxy/envoy/issues/1930 for cluster warming.
On Wed, Feb 14, 2018 at 5:04 PM Kuat Yessenov<EMAIL_ADDRESS>wrote:

Shriram just explained it, but there are at least two problems with this
approach:

RDS needs to be blocked on EDS update, not just CDS update which is not
possible
What happens when clusters are removed?

On Wed, Feb 14, 2018 at 5:02 PM Andra Cismaru<EMAIL_ADDRESS>wrote:

I checked with Envoy and this is what we are supposed to do, this is the
correct Envoy behaviour. We should have done this already. Do you have some
arguments to support why this is not a good idea?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365795866, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxpI21W4QIDX7x5fexvCzgnFix6chks5tU4I0gaJpZM4SEx-i
.

that's good points to further explore and refine, but what is the exact downside of that flag (if it works and it does convert even some classes of errors to non errors) applied incrementally ?

Let's test this - if it does indeed reduces the 503 ( I don't think we have 404s any more ) - it's great.
That is what Harvey and Piotr suggested - we may do other things to improve further.

The downside is more non-determinism since now RDS updates are denied if
they come prior to CDS. Since clusters can be both added and removed at the
same time, I'm not sure the thing will converge fast enough across CDS and
RDS update sequence.
On Wed, Feb 14, 2018 at 5:10 PM Laurent Demailly<EMAIL_ADDRESS>wrote:

that's good points to further explore and refine, but what is the exact
downside of that flag (if it works and it does convert even some classes of
errors to non errors) applied incrementally ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365797016, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxtIdo_UUiOnESYCOj2b7nBhwyH2Kks5tU4PogaJpZM4SEx-i
.

How do we qualify the testing? Do we have a reproducible set-up so that we
can make a data-driven decision? This has both positive and negative sides,
so we need to evaluate the trade-off.
On Wed, Feb 14, 2018 at 5:11 PM Kuat Yessenov<EMAIL_ADDRESS>wrote:

The downside is more non-determinism since now RDS updates are denied if
they come prior to CDS. Since clusters can be both added and removed at the
same time, I'm not sure the thing will converge fast enough across CDS and
RDS update sequence.
On Wed, Feb 14, 2018 at 5:10 PM Laurent Demailly<EMAIL_ADDRESS>wrote:

that's good points to further explore and refine, but what is the exact
downside of that flag (if it works and it does convert even some classes of
errors to non errors) applied incrementally ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365797016, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxtIdo_UUiOnESYCOj2b7nBhwyH2Kks5tU4PogaJpZM4SEx-i
.

for historical context, here is the chat on envoy-dev I had with Matt in october:
laurent [3:53 PM]
we kind of need a quick fix for those 404s when rds comes before cds, any creative suggestions ? (edited)
(that doesn't involve switching to grpc and ads)
maybe some way for envoy to verify routes are not dead ends and ignore the ones that are dead ends (or trigger a synchronous rds refresh when found such dead end)

mklein [3:56 PM]
put a hack in pilot to add a sleep or something
between refreshes
or sequence them
with sleeps
is that possible?

laurent [3:57 PM]
partially, it doesn't cover the case where we have multiple pilots and one of them is lagging behind in getting updates from k8s
also it 100% delays updates by 1 cycle when the race is client side

mklein [3:58 PM]
“also it 100% delays updates by 1 cycle when the race is client side” what does that mean?

laurent [3:59 PM]
if lucky, the operator makes a change, the changes propagate in the next refresh cycle (say 10s) in the right order
on average it takes 5s for changes to show up with x% of case where it's broken for another 10s
if we delay it'll always take 15s or such for all changes

mklein [4:00 PM]
I’m not sure there is any easy solution here other than switching to ADS if you need atomic/sequenced updates

laurent [4:01 PM]
do you think we can't hack (in our branch maybe if needed) a "validation" to rds that checks the cds is there and spew unhappyness in the logs about bad routes being ignored for now, instead of 404s to the user ? (edited)
I have not looked at envoy code or architecture so my apologies if what I say is complete nonsense
asking for your advice on anything creative that would be quick (as a side note someone here mentioned it should be 503 and not 404 to avoid end user app making a decision that something is truly gone)

mklein [4:03 PM]
unfortunately it doesn’t really work that way. All updates inside Envoy are atomic. There is no going back
by the time you know it’s bad, there is nothing to go back to

laurent [4:04 PM]
if the json is not formatted well; you don't apply it right ? so can't that validation do a bit more semantic-ish validation (even if the change can be the other direction, the cds could disappear, but just trying to address the most common, observed issue not cover 100% correctness) (edited)

mklein [4:05 PM]
ah
yes
there is a hack here you can do
one sec and I have you covered
brb

laurent [4:05 PM]
woot - thx, take your time.

mklein [4:08 PM]
@laurent see here: https://envoyproxy.github.io/envoy/configuration/http_conn_man/route_config/route_config.html#config-http-conn-man-route-table
set “validate_clusters” to true in your response
for RDS it defaults to false
if you set to true, envoy will reject update
and will keep using current route table

laurent [4:09 PM]
oh cool ! it's already there ! fantastic
let me get a test repro'ing the user report and then try this, that's awesome

If it reduces / eliminates 503, without breaking routes - it's a good change.
We have eventual consistency - some delays are ok, next RDS update will take effect.
For now - this seem the best way to improve the 503 situation. It may not be perfect,
and we may need additional changes in envoy or pilot for 0.7 (for example delay the
RDS on pilot side) - but I don't see any clear harm.
Why wouldn't we validate_cluster ? If the cluster is invalid/missing, what's the point in accepting the route ?

Yes, Matt's advice to sequence RDS update is also possible if this one doesn't solve all 503.
We can easily delay the clearCache for routes - it's better to have correct behavior slower
than 503s.

How is it a fix if it does not solve the problem of inconsistent xDS config?

If after this change we don't see 503 errors - it is a fix for the 503 bug, which affects real users.
I don't know what other problem you have regarding xDS - it is eventually consistent, so a future
RDS update will get the new route.

Kuat: have you tested with this option on and noticed any failure ?
I understand you believe ADS is the only possible solution - but if this or sequencing
the RDS/CDS improves things, I think it is still a win and there is no valid reason to block this.

Please provide some evidence this has positive effect and a flag to control the behavior. I would also want to limit this to ingress since mesh-internal traffic we have client under control.

Mesh internal traffic can also see invalid clusters.
Laurent has a test reproducing 503 - so we'll test with it. If the fix solves it - I don't think
we need a 'return 503' (or invalid_clusters) flag is really needed.

I think this deserves a flag since it can have some serious consequences. I'm thinking that envoy-to-envoy there are better ways to hide these 503s.

Keep in mind that 503/404 needs to be tested with a 60s refresh delay. If
you are testing with a 1s delay, you don’t even need this optimization.
On Wed, Feb 14, 2018 at 8:33 PM Kuat<EMAIL_ADDRESS>wrote:

I think this deserves a flag since it can have some serious consequences.
I'm thinking that envoy-to-envoy there are better ways to hide these 503s.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365800872, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AH0qd1PAjcYQK9YWhfJY9bINv219CyZfks5tU4mHgaJpZM4SEx-i
.

for anyone who want to test/compare : this PR's pilot:
        image: gcr.io/istio-testing/pilot:31c44885a0825713110bc92c5b6d85235e26a767

vs
        image: gcr.io/istio-release/pilot:0.6.0-pre20180213-09-15-00

I'm about to test it with #1041

bad news: it doesn’t help/work (see #1041) :(

Ok, let's add a flag, and investigate why it doesn't work and how we can
adjust the CDS/RDS to
make it work.
Either:

Envoy applied the route despite not having valid clusters, instead of
keeping the old one
envoy kept the old route, but the old route no longer has the clusters
because the CDS update
changed the names.

The label-based selection works by generating new cluster names.
Plan B is (as Matt also suggested) to time the updates, but while we
explore this we should
also investigate if envoy has a bug in the cluster validation.
On Wed, Feb 14, 2018 at 7:49 PM, Laurent Demailly<EMAIL_ADDRESS>wrote:

bad news: it doesn’t help/work (see #1041
https://github.com/istio/istio/pull/1041) :(
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365818274, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFI6jyQWSmjE6hvAyU1u5HFlq2Y6Miyks5tU6k3gaJpZM4SEx-i
.

Ok, since this doesn't fix the bug, let's at least put a feature gate in
order not to destabilize everything else.
@costinm I already said it twice and Shriram once, but this is because EDS
update does not come on time.
On Wed, Feb 14, 2018 at 8:10 PM Costin Manolache<EMAIL_ADDRESS>wrote:

Ok, let's add a flag, and investigate why it doesn't work and how we can
adjust the CDS/RDS to
make it work.
Either:

Envoy applied the route despite not having valid clusters, instead of
keeping the old one
envoy kept the old route, but the old route no longer has the clusters
because the CDS update
changed the names.

The label-based selection works by generating new cluster names.
Plan B is (as Matt also suggested) to time the updates, but while we
explore this we should
also investigate if envoy has a bug in the cluster validation.
On Wed, Feb 14, 2018 at 7:49 PM, Laurent Demailly <
<EMAIL_ADDRESS>wrote:

bad news: it doesn’t help/work (see #1041
https://github.com/istio/istio/pull/1041) :(
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365818274, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AAFI6jyQWSmjE6hvAyU1u5HFlq2Y6Miyks5tU6k3gaJpZM4SEx-i
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365820419, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxs6i7wbyN95uI5YIG5qx_Tl2fdYzks5tU64dgaJpZM4SEx-i
.

So:

RDS comes in with cluster xyz
envoy validate that it got a CDS, and has the cluster xzy
envoy attempts to connect to cluster xyz to satisfy the request
pilot doesn't know about xyz ?

I think next step is to add request logging to pilot to confirm the sequence (for some reason
the http requests from envoy are not logged), and find out why EDS doesn't have endpoints.
It seems possible to introduce delays in both RDS and CDS updates.
I agree we can add a flag, to allow user to chose very fast update (but 503) or
slower updates without 503.
Simplest way to test this theory would be to delay the cache clearing on RDS/CDS.

It may also be worth testing with v1alpha2 - and first apply destination rule, and only after route rules.
Since we plan to eventually deprecate v1alpha1, and the new API allows a clear separation of
subset definitions ( affecting EDS/CDS) from rules - it might be ok to recommend switching to alpha2
for anyone affected.

let's figure out what it does on the envoy side - maybe we're missing some other setting (or there is a bug in envoy)
I'd be very concerned about adding delays or state on pilot side trying to keep track which client asked what when, that will not scale

Delays on pilot don't even work with multiple replicas. I feel like we went
through several times already.
Re: v1alpha2, you have another problem there with lack of referential
integrity in k8s, which is entirely unexplored. You can get a route prior
to a cluster, or one pilot being behind another pilot in getting clusters.
On Wed, Feb 14, 2018 at 9:08 PM Laurent Demailly<EMAIL_ADDRESS>wrote:

let's figure out what it does on the envoy side - maybe we're missing some
other setting (or there is a bug in envoy)
I'd be very concerned about adding delays or state on pilot side trying to
keep track which client asked what when, that will not scale
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365826834, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxiy3ZQHBjuXXG-OAtpdcPXtyZwYAks5tU7vGgaJpZM4SEx-i
.

@rshriram This is not a hack, it is meant to not drop traffic, but rather send it to previous good clusters. If the RDS config contains two weighted clusters C1 and C2, but CDS returns only C2 and validate_cluster is not set, Envoy will drop traffic destined to C1. If validate_cluster is set, Envoy will continue to operate until both C1 and C2 are defined.
@kyessenov yes, this introduces a delay until a route rule becomes effective, <= refresh rate. This is a preferable trade off to having dropped traffic and 503s.
We can introduce a flag to allow the user to choose one bad over the other, but this is not a priority, since I have not heard anybody complaining about how long it takes for RR to be effective, and it can be tuned by choosing a different refresh interval.
I prefer to focus on investigating why Laurent's test does not pass, it seems there are additional changes needed.

@rshriram When you say endpoints are not there in the cluster, is it because EDS may not have arrived?
@PiotrSikora after receiving a CDS response, does Envoy make an EDS requests, or it waits for traffic firsts? Does Envoy always configure endpoints even if there is no traffic?

@rshriram if I understand correctly, your concern with this PR is that it does not solve the problem entirely. It solves the cluster validity, but not the fact that clusters may have zero endpoints for a period of time, in which case it will still return 503. I am thinking 503 service unavailable is somehow justified in this case, since it is triggered by a routing rule change, though we may possibly prevent it completely. We are trying to provide here with this PR is a path towards hitless routing rule changes and we should merge this change anyway, because it helps with scalability.
@costinm The period of time for 503 with this change should be reduced, it should only be the timeframe between receiving the CDS response and accepting the clusters, and receiving the EDS response. It does not depend on the refresh interval, because EDS is triggered by the CDS response. Can you try your scale test with my image gcr.io/istio-testing/pilot:andracis-503?

Full solution for a hitless Routing Rule update could be to change Envoy to not accept a cluster with zero endpoints, when validate_clusters is set.
There is no sequencing Pilot can do to solve the zero endpoints problem, and v2alpha would not solve it either.
With the 503 replacing 404, and the validate_clusters reducing the period of time for 503 from something > refresh_interval to something minimal, this entire issue is less impactful.
I suggest we merge this PR, and defer the hitless RR updates for when ADS is available (unless Envoy agrees to make the change above).

Regarding timing: AFAIK K8s rules have a timestamp when they were last modified, so it is possible to
ignore/delay the RDS part.
I understand that ADS may solve all problems - but that doesn't mean it's the only solution, or we should
stop partial improvements.
I agree 'validate' cluster is likely incorrect if it treats clusters that exist but don't have endpoints as valid, and should be fixable in envoy. But since the root problem is timing of changes - ordering and timing the changes in pilot should work as well as ADS, where pilot also controls order of EDS/CDS/RDS, but in a different way.

Also on EDS missing problem - agree it is likely the cause, however:

this is something we can try to fix in envoy
this PR still solves 1/2 of the problem, i.e. the case RDS is received before CDS. Which is not bad.

Timing in docker is unreliable due to virtualization, please consider that.
I've linked cluster warming issue in my first comment. That's the issue
with empty valid clusters.
This PR introduces a different behavior which needs to be evaluated. Can
you gate it with a flag?
On Thu, Feb 15, 2018 at 5:20 PM Costin Manolache<EMAIL_ADDRESS>wrote:

Also on EDS missing problem - agree it is likely the cause, however:

this is something we can try to fix in envoy
this PR still solves 1/2 of the problem, i.e. the case RDS is
received before CDS. Which is not bad.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366117609, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxnF419DTssmqokuRU99IrJRFcDsOks5tVNfOgaJpZM4SEx-i
.

I suggest we merge this PR

what does it change ? there should be at least a manual test or some indication of benefit, no ?
(and ideally a full test like #1041 passing)
did you get a chance to grep/search for validate_clusters in envoy's code and tests ?

Recap: this is v1, and there is no ADS available soon, and still the problem needs to be fixed.
To guarantee hitless routing rule changes (no 503s whatsoever), we need that any config change committed in Envoy to contain valid clusters (1) with at least one endpoint (2) (assuming the service has indeed at least one healthy endpoint).

Valid cluster part is solved by validate_cluster in this PR.
The at least one endpoint problem can be solved deterministically only by changing Envoy, there is nothing pilot can do. Sequencing pilot's REST responses would not guarantee the order in which Envoy receives them, since we are in a distributed system. The most reasonable change in Envoy is cluster warming, as proposed by @htuch (thanks for all the insights) and by @kyessenov:
https://github.com/envoyproxy/envoy/issues/1930
The two above combined will ensure Envoy operates with a valid config, equivalent to ADS.

Meanwhile, and until the Envoy fix is available, we can advise users to use the workaround that @costinm proposed, which consists in switching to v2alpha RR API, which forces the user to configure and name the clusters, prior to configuring routing rules. The operator workflow will be:

Configure EndpointSubsets (equivalent of Envoy Clusters)
Wait a time period >  refresh rate ~10-30 minutes (@louiscryan is this acceptable)
Apply the actual Routing Rules.

I will try to adapt @ldemailly 's test to use v2alpha and sleep between configuring clusters and applying the Routing Rules (@ldemailly thanks for writing this and trying it).
In all cases, validate_clusters is needed, unless somebody provides a use case where Envoy needs to operate with an invalid cluster, and operate without producing 404s or 503s. I could add a flag just to help with people's sense of confidence, not because I find this request is justified.

https://github.com/istio/istio/pull/1041/files#diff-be79bfbce28d518ddd73d0131f701496R155-R195
does multiple changes and gather combined response codes, in part to increase the chance to trigger the 503s. I suggest you take that test and indeed modify it in any way you see fit that shows (across multiple runs as it doesn't always trigger) that you get noticeably less 503s with this PR than without. Once that is done I'm sure everyone will be glad this gets merged and reduces the 503s in anyway.
But we can't just hope it helps when experimentally so far it doesn't seem to. It's quite likely that the lack of effect isn't that the test is bad but that the feature doesn't work the way we expect in envoy and we need to dig deeper.
You can merge the test on this and we turn it into a 503s fix feature branch - until it does show some measurable improvement ?

One idea could be to accelerate EDS/CDS frequency vs RDS/LDS frequencies.
It's easy to speed up EDS/CDS computation vs RDS/LDS since most of the data
is shared across proxies. @costin since you changed the xDS cluster
definitions, can you look into splitting back these services so we can play
with that (I remember you merged them all into RDS for some reason).
On Wed, Feb 14, 2018 at 8:49 PM Costin Manolache<EMAIL_ADDRESS>wrote:

It may also be worth testing with v1alpha2 - and first apply destination
rule, and only after route rules.
Since we plan to eventually deprecate v1alpha1, and the new API allows a
clear separation of
subset definitions ( affecting EDS/CDS) from rules - it might be ok to
recommend switching to alpha2
for anyone affected.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365824709, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxm7vc_zhJKyhwLQ2DoJXFg9zNKlIks5tU7dVgaJpZM4SEx-i
.

I think the success criteria should include a test with multiple pilots, as
that's the expected production deployment. Some of the proposed solutions
fall apart with multiple discoordinated pilots (using time is not an option
unfortunately). I think the accelerated EDS/CDS poll frequency might help
in this case to reduce 503/404s, but not completely eliminate them. For
that we need, cluster warming and also work through deletion scenarios, not
just addition.
On Fri, Feb 16, 2018 at 7:37 PM Kuat Yessenov<EMAIL_ADDRESS>wrote:

One idea could be to accelerate EDS/CDS frequency vs RDS/LDS frequencies.
It's easy to speed up EDS/CDS computation vs RDS/LDS since most of the data
is shared across proxies. @costin since you changed the xDS cluster
definitions, can you look into splitting back these services so we can play
with that (I remember you merged them all into RDS for some reason).
On Wed, Feb 14, 2018 at 8:49 PM Costin Manolache<EMAIL_ADDRESS>wrote:

It may also be worth testing with v1alpha2 - and first apply destination
rule, and only after route rules.
Since we plan to eventually deprecate v1alpha1, and the new API allows a
clear separation of
subset definitions ( affecting EDS/CDS) from rules - it might be ok to
recommend switching to alpha2
for anyone affected.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-365824709, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxm7vc_zhJKyhwLQ2DoJXFg9zNKlIks5tU7dVgaJpZM4SEx-i
.

Getting 503s for 30s is not a good solution over getting 404s for 30s (30s being the refresh interval). If you are going to set the refresh rate to 1s, this problem is moot.
The whole idea behind ads was to solve this problem by sequencing the updates over a single connection.
Also, validate clusters is a one way hack (yes it’s still a hack). All the discussion here focuses on something like one cluster becoming two clusters (v1,v2). Think about what would happen in the reverse ? If I remove the route rule or change it to have no weighted routing, you now have a different problem. If my CDs update goes before my RDS update, and only contains a single cluster for the service, then for a time period X, you are going to get 404s until the respective RDS goes in. You can’t solve this by never deleting CDs clusters as that will result in unbounded growth and more inconsistencies.

@rshriram Getting 503 (service unavailable) is preferable to getting 404 (not found). For the former, the client can retry. The latter means the service has been deleted. Think of your calendar invite to  being gone. This was already fixed in Envoy, that was part 0 of the full solution. Validate_clusters reduces the 503 interval from the refresh rate, to something smaller, at least in theory. Please read my previous comments.
Using 1s refresh interval is not a viable solution in a scale setup.
ADS does not solve the problem by sequencing, but by making the update atomic.

@rshriram It will work for deletion too, because it is not a cluster C becoming C1 and C2, C will continue to be there when C1 and C2 are added. For v2alpha:
We start with cluster C which contains all endpoints in the service. CDS sends C, RDS points to C.
Create:

We create config with 2 EndpointSubsets C1 and C2. Pilot sends CDS with C, C1, C2. RDS will point to C, no traffic loss.
wait refresh interval (can be removed when Envoy does cluster warming )
Create config RR weighted between C1 and C2 -> Pilot send RDS with clusters C1 and C2; clusters are there so no traffic loss. C is not used

Delete

delete config RR -> Pilot sends RDS with C, which exists. C1 and C2 are not used
Delete config for EndpointSubsets --> CDS deletes clusters C1 and C2, C is still there, no traffic loss

[...] It will work [...]

There is not much point in arguing about the theory on the pr, I think that energy would be better spent running tests and finding the issue / what this changes or doesn't change and why and report findings and data instead ?

This is a design discussion. We have to come up with a design that is theoretically correct, otherwise going straight to testing may show false positives. Shriram's concerns about deletion are valid, and we may need extra changes, because now we do not send the the main cluster C, only C1 or C2.

please start a design doc or an issue then. this pr is about whether validate_cluster helps/works

This is true, this PR is about validate_true, and we have to favor incremental bite-size PRs, and not request everything be bundled in a huge PR.
Validate_true does not negatively impact the delete scenario, as one could infer by reading @rshriram 's comment. Deletion is not a use case that proves there is a problem with enabling validate_cluster, but it had to be analysed.

Validate only works one way as far as I know. If you remove a cluster,
there's no additional validation on existing routes, so you would get
errors then too.
On Sun, Feb 18, 2018, 8:51 AM Andra Cismaru<EMAIL_ADDRESS>wrote:

This is true, this PR is about validate_true, and we have to favor
incremental bite-size PRs, and not request everything be bundled in a huge
PR.
Validate_true does not negatively impact the delete scenario, as one could
infer by reading @rshriram https://github.com/rshriram 's comment.
Deletion is not a use case that proves there is a problem with enabling
validate_cluster, but it had to be analysed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366529333, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxpLzuLYvetVeEmf0MdSZiO2OJKt7ks5tWFUHgaJpZM4SEx-i
.

If you remove c2 and add a c3, what happens ? (Common cluster C is a special case).

Folks, I think we all understand at a low level why and when Envoy returns 503 / 404.
I am looking for a set of customer scenarios that are affected by this 503 and specifically those that cannot be addressed by sequencing config changes thru CI/CD.  Things like force create before use, etc.
Depending on the customer, the universe of clusters and cluster dimensions may be known before hand and very slowly changing. I want to understand for which type of customer the current situation is completely inadequate.

Mandar,
The challenge is to solve the 404/503 with minimal changes to the existing
code. We know that ADS works and solves this problem. I agree that
subset_lb does improve the situation, but it has a significant effect on
other parts.
On Mon, Feb 19, 2018 at 8:22 PM mandarjog<EMAIL_ADDRESS>wrote:

Subset_lb should mitigate this situation quite a bit, since there will
only be one cluster per destination and everything else is dynamically
partitioned as a subset. At present subset_lb requires us to have pre
defined dimensions, but that looks like an implementation choice.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366863170, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxqIsHOXxg5b8lhM1f-Qu4s2OQGUlks5tWkhsgaJpZM4SEx-i
.

As a reminder, we didn't have 503 in Istio 0.1. We made an implementation
choice - on how to generate the clusters
and how to propagate the config.
ADS may be one solution - and we may use it we are in process of
implementing the v2 push and after
we migrate them all ADS is one extra step. That's a different discussion -
I hope nobody believe we can
satisfy the schedule and have users wait for moving pilot to ADS, or expect
ADS will not take quite a bit
of time to stabilize for all the features we have.
However it is not productive to argue that because there is a corner case
where things won't work

we shouldn't make any improvements. Computers may have time out of sync -
but in most
cases NTP does a good enough job, and sequencing the  EDS, CDS and RDS is
perfectly possible.

Pilot knows the timestamp when a cluster was created, and has the timestamp
when the route are created,
and in almost all cases can send routes only after clusters and endpoints.
Even if multiple pilots
are used - the time drift across pilots can be handled, and it appears
envoy even supports some stickiness
in the clusters ( which will also benefit cache hit - so we'll want to use
it anyways )
So:

Envoy is fixing CDS ( and it appear that may happen in 0.7 if we focus ),
which will let 'validate_clusters'
work properly. This remains the master plan.

we can also test sequencing, using the timestamp of the RDS change. May
not solve 100% of cases, but since
envoy recommends CDS to be sent first and for clusters to be stable before
sending traffic - I see no downside.
We can adjust the values - and let the users chose the tradeoff. We're
already introducing some timing into
the system - to allow it to scale. As we move to push APIs and new data
structs - we'll have far more flexibility,
so even if we don't pursue this now, we should pay attention to it in the
new code.

There are always multiple solutions - and different trade-offs. It's not so
hard to revert to 0.1 behavior, and have
the agent download all the pieces into a static LDC/RDS/CDS config.
Probably not worth given that it's likely
envoy changes will solve the problem.
On Mon, Feb 19, 2018 at 8:29 PM, Kuat<EMAIL_ADDRESS>wrote:

Mandar,
The challenge is to solve the 404/503 with minimal changes to the existing
code. We know that ADS works and solves this problem. I agree that
subset_lb does improve the situation, but it has a significant effect on
other parts.
On Mon, Feb 19, 2018 at 8:22 PM mandarjog<EMAIL_ADDRESS>wrote:

Subset_lb should mitigate this situation quite a bit, since there will
only be one cluster per destination and everything else is dynamically
partitioned as a subset. At present subset_lb requires us to have pre
defined dimensions, but that looks like an implementation choice.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366863170, or
mute
the thread
<https://github.com/notifications/unsubscribe-
auth/AJGIxqIsHOXxg5b8lhM1f-Qu4s2OQGUlks5tWkhsgaJpZM4SEx-i>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366864036, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFI6iyjlEBfYLldvxTA7Kuv6dJYDyDNks5tWkpCgaJpZM4SEx-i
.

I'm not sure that's the case that 0.1 did not have this problem. From what
I remember, the logic to create clusters and routes has been identical ever
since the beginning.
On Mon, Feb 19, 2018 at 10:00 PM Costin Manolache<EMAIL_ADDRESS>wrote:

As a reminder, we didn't have 503 in Istio 0.1. We made an implementation
choice - on how to generate the clusters
and how to propagate the config.
ADS may be one solution - and we may use it we are in process of
implementing the v2 push and after
we migrate them all ADS is one extra step. That's a different discussion -
I hope nobody believe we can
satisfy the schedule and have users wait for moving pilot to ADS, or expect
ADS will not take quite a bit
of time to stabilize for all the features we have.
However it is not productive to argue that because there is a corner case
where things won't work

we shouldn't make any improvements. Computers may have time out of sync -
but in most
cases NTP does a good enough job, and sequencing the EDS, CDS and RDS is
perfectly possible.

Pilot knows the timestamp when a cluster was created, and has the timestamp
when the route are created,
and in almost all cases can send routes only after clusters and endpoints.
Even if multiple pilots
are used - the time drift across pilots can be handled, and it appears
envoy even supports some stickiness
in the clusters ( which will also benefit cache hit - so we'll want to use
it anyways )
So:

Envoy is fixing CDS ( and it appear that may happen in 0.7 if we focus ),
which will let 'validate_clusters'
work properly. This remains the master plan.

we can also test sequencing, using the timestamp of the RDS change. May
not solve 100% of cases, but since
envoy recommends CDS to be sent first and for clusters to be stable before
sending traffic - I see no downside.
We can adjust the values - and let the users chose the tradeoff. We're
already introducing some timing into
the system - to allow it to scale. As we move to push APIs and new data
structs - we'll have far more flexibility,
so even if we don't pursue this now, we should pay attention to it in the
new code.

There are always multiple solutions - and different trade-offs. It's not so
hard to revert to 0.1 behavior, and have
the agent download all the pieces into a static LDC/RDS/CDS config.
Probably not worth given that it's likely
envoy changes will solve the problem.
On Mon, Feb 19, 2018 at 8:29 PM, Kuat<EMAIL_ADDRESS>wrote:

Mandar,
The challenge is to solve the 404/503 with minimal changes to the
existing
code. We know that ADS works and solves this problem. I agree that
subset_lb does improve the situation, but it has a significant effect on
other parts.
On Mon, Feb 19, 2018 at 8:22 PM mandarjog<EMAIL_ADDRESS>wrote:

Subset_lb should mitigate this situation quite a bit, since there will
only be one cluster per destination and everything else is dynamically
partitioned as a subset. At present subset_lb requires us to have pre
defined dimensions, but that looks like an implementation choice.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366863170, or
mute
the thread
<https://github.com/notifications/unsubscribe-
auth/AJGIxqIsHOXxg5b8lhM1f-Qu4s2OQGUlks5tWkhsgaJpZM4SEx-i>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366864036, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AAFI6iyjlEBfYLldvxTA7Kuv6dJYDyDNks5tWkpCgaJpZM4SEx-i
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366874925, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJGIxgT790c_9R6XQ9W26NWc8MzE-lbuks5tWl9sgaJpZM4SEx-i
.

In 0.1 we generated same clusters, but they were saved to disk in one
consistent config.
The problem was the frequent envoy restart ( it was restarting on each
change).
Now in v2 there is support for filesystem configs watched with inotify. So
the agent could
orchestrate the timing - with the extra major benefit of better handling
failures.
Since agent already gets AZ from pilot - it's not hard to extend this to
get the other configs,
and I believe the config source supports REST_LEGACY format as well. We
should give it
a try - will also be very easy to debug and might be a great match for
making it more
configurable (like the webhook trick Shriram did on server side).
On Mon, Feb 19, 2018 at 10:03 PM, Kuat<EMAIL_ADDRESS>wrote:

I'm not sure that's the case that 0.1 did not have this problem. From what
I remember, the logic to create clusters and routes has been identical ever
since the beginning.
On Mon, Feb 19, 2018 at 10:00 PM Costin Manolache <
<EMAIL_ADDRESS>wrote:

As a reminder, we didn't have 503 in Istio 0.1. We made an implementation
choice - on how to generate the clusters
and how to propagate the config.
ADS may be one solution - and we may use it we are in process of
implementing the v2 push and after
we migrate them all ADS is one extra step. That's a different discussion

I hope nobody believe we can
satisfy the schedule and have users wait for moving pilot to ADS, or
expect
ADS will not take quite a bit
of time to stabilize for all the features we have.
However it is not productive to argue that because there is a corner case
where things won't work

we shouldn't make any improvements. Computers may have time out of
sync -
but in most
cases NTP does a good enough job, and sequencing the EDS, CDS and RDS is
perfectly possible.

Pilot knows the timestamp when a cluster was created, and has the
timestamp
when the route are created,
and in almost all cases can send routes only after clusters and
endpoints.
Even if multiple pilots
are used - the time drift across pilots can be handled, and it appears
envoy even supports some stickiness
in the clusters ( which will also benefit cache hit - so we'll want to
use
it anyways )
So:

Envoy is fixing CDS ( and it appear that may happen in 0.7 if we focus
),
which will let 'validate_clusters'
work properly. This remains the master plan.

we can also test sequencing, using the timestamp of the RDS change. May
not solve 100% of cases, but since
envoy recommends CDS to be sent first and for clusters to be stable
before
sending traffic - I see no downside.
We can adjust the values - and let the users chose the tradeoff. We're
already introducing some timing into
the system - to allow it to scale. As we move to push APIs and new data
structs - we'll have far more flexibility,
so even if we don't pursue this now, we should pay attention to it in the
new code.

There are always multiple solutions - and different trade-offs. It's not
so
hard to revert to 0.1 behavior, and have
the agent download all the pieces into a static LDC/RDS/CDS config.
Probably not worth given that it's likely
envoy changes will solve the problem.
On Mon, Feb 19, 2018 at 8:29 PM, Kuat<EMAIL_ADDRESS>wrote:

Mandar,
The challenge is to solve the 404/503 with minimal changes to the
existing
code. We know that ADS works and solves this problem. I agree that
subset_lb does improve the situation, but it has a significant effect
on
other parts.
On Mon, Feb 19, 2018 at 8:22 PM mandarjog<EMAIL_ADDRESS>wrote:

Subset_lb should mitigate this situation quite a bit, since there
will
only be one cluster per destination and everything else is
dynamically
partitioned as a subset. At present subset_lb requires us to have pre
defined dimensions, but that looks like an implementation choice.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366863170,
or
mute
the thread
<https://github.com/notifications/unsubscribe-
auth/AJGIxqIsHOXxg5b8lhM1f-Qu4s2OQGUlks5tWkhsgaJpZM4SEx-i>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366864036, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/
AAFI6iyjlEBfYLldvxTA7Kuv6dJYDyDNks5tWkpCgaJpZM4SEx-i
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366874925, or
mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJGIxgT790c_
9R6XQ9W26NWc8MzE-lbuks5tWl9sgaJpZM4SEx-i>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-366875318, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFI6vKtbW09XrIpm7HORidKIgewKE_Bks5tWmAjgaJpZM4SEx-i
.

Envoy is fixing CDS

issue/pr # ? fixing what ?
and again can the overall plan be put in a doc or issue as it appears quite involved

+1 @ldemailly, would like to continue this discussion in a doc, there are a number of distinct threads to address.

We can start a doc on the timing or CDS warming - however on this PR, is
there any reason to block
it ?

Is there any case where we would want to accept a route - when there is
no valid cluster ?
Even if envoy implementation of 'validate_cluster' is not working fully,
is this the correct setting to
send to envoy, when we want envoy to not accept routes with missing
clusters ?

Understood that it doesn't currently solve the problem - but if it is the
correct way to configure envoy
I don't want to hold it, or to have everyone who's going to work on this to
cherry-pick this PR.
On Tue, Feb 20, 2018 at 7:42 AM, htuch<EMAIL_ADDRESS>wrote:

+1 @ldemailly https://github.com/ldemailly, would like to continue this
discussion in a doc, there are a number of distinct threads to address.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-367018510, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFI6qqE8EE89O0yBgXWHDo34q3JA-Unks5tWufVgaJpZM4SEx-i
.

And also: do we need a document or meeting to discuss if "validate_cluster"
( even
if not fully implemented in envoy ) is the proper config to generate, or is
envoy's
doc enough ?
On Tue, Feb 20, 2018 at 8:48 AM, Costin Manolache<EMAIL_ADDRESS>wrote:

We can start a doc on the timing or CDS warming - however on this PR, is
there any reason to block
it ?

Is there any case where we would want to accept a route - when there is
no valid cluster ?
Even if envoy implementation of 'validate_cluster' is not working
fully, is this the correct setting to
send to envoy, when we want envoy to not accept routes with missing
clusters ?

Understood that it doesn't currently solve the problem - but if it is the
correct way to configure envoy
I don't want to hold it, or to have everyone who's going to work on this
to cherry-pick this PR.
On Tue, Feb 20, 2018 at 7:42 AM, htuch<EMAIL_ADDRESS>wrote:

+1 @ldemailly https://github.com/ldemailly, would like to continue
this discussion in a doc, there are a number of distinct threads to address.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-367018510, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFI6qqE8EE89O0yBgXWHDo34q3JA-Unks5tWufVgaJpZM4SEx-i
.

A quick meeting would work, feel free to schedule any time free on my calendar that makes sense EST @costinm, thanks.

As mentioned several times, this pr is waiting because of lack of data showing it works/does anything at all. How are we doing on that ? and on checking envoy side implementation and tests of the flag ?

Also again while this is OT for the PR and should be in some doc or issue, can you please answer

Envoy is fixing CDS

Fixing what ? Is there an issue/pr # and/or link to email, etc ?

We confirmed this PR doeesn't fix the 503, due to missing code in envoy -
but is appears to be the
correct configuration.
Will discuss with Envoy eng on their side. We don't typically require a doc
or issue for each PR.
On Tue, Feb 20, 2018 at 9:28 AM, Laurent Demailly<EMAIL_ADDRESS>wrote:

As mentioned several times, this pr is waiting because of lack of data
showing it works/does anything at all. How are we doing on that ? and on
checking envoy side implementation and tests of the flag ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-367053814, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFI6jVE1qaHzrutl1JoVbTmmcEjvZkCks5tWwDMgaJpZM4SEx-i
.

@ldemailly The corresponding fix in Envoy is here - support for CDS cluster "warming", which ensure clusters with zero endpoints are not committed:
https://github.com/envoyproxy/envoy/issues/1930
@htuch Will book a meeting to get guidance, let me check with @louiscryan

The 'lazy loading' bug referenced is also very interesting for us for scale
and
multi-cloud (very large meshes).
But back to the PR: except for the request to add a flag (or environment
variable, since
it's likely to be short-lived and doesn't need to be exposed/documented to
users), and the
concerns that this is not a full solution - are there any reasons not to
approve ?
On Tue, Feb 20, 2018 at 3:58 PM, Andra Cismaru<EMAIL_ADDRESS>wrote:

@ldemailly https://github.com/ldemailly The corresponding fix in Envoy
is here - support for CDS cluster "warming", which ensure clusters with
zero endpoints are not committed:
envoyproxy/envoy#1930 https://github.com/envoyproxy/envoy/issues/1930
@htuch https://github.com/htuch Will book a meeting to get guidance,
let me check with @louiscryan https://github.com/louiscryan
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/3482#issuecomment-367164191, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFI6jP7_REdQDTgJvYb3W1HSbRB-kX2ks5tW1wogaJpZM4SEx-i
.

@andraxylia: The following test failed, say /retest to rerun them all:

Test name
Commit
Details
Rerun command

prow/istio-unit-tests.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test istio-unit-tests

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@andraxylia: The following tests failed, say /retest to rerun them all:

Test name
Commit
Details
Rerun command

prow/istio-unit-tests.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test istio-unit-tests

prow/e2e-simpleTests.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test e2e-simple

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@andraxylia: The following tests failed, say /retest to rerun them all:

Test name
Commit
Details
Rerun command

prow/istio-unit-tests.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test istio-unit-tests

prow/e2e-simpleTests.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test e2e-simple

prow/e2e-bookInfoTests.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test e2e-bookInfo

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Closing in favor of #4024 , since the set of files changed got messed up by the merge.

I don't see validate_cluster=true in #4024 (which seems to be about the timeouts?)

@andraxylia: The following tests failed, say /retest to rerun them all:

Test name
Commit
Details
Rerun command

prow/istio-unit-tests.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test istio-unit-tests

prow/e2e-simpleTests.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test e2e-simple

prow/e2e-bookInfoTests.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test e2e-bookInfo

prow/istio-pilot-e2e.sh
65622f8bb7aef1852f247ac7f6b41a18d7b986ad
link
/test istio-pilot-e2e

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Rebase messed up the set of files, closing this PR and opening a new one from the same branch #4031

looks like it's #4039 now


refactor: be able to run app in production environment locally with ease

This is done, look for how to in readme


Add options to effects/stop

As pointed out in #61, Need to add to effects/stop options so you can pass things like power_on to the SetTileEffect and SetZonesEffect objects it uses

From what I can tell, power_on is about the only useful option for effects/stop? Would it not be easier just to add that as a boolean option?

Though, having a power_off option for effects/stop would be rather useful too, I reckon.

yeah, atm power_on is only useful one probably. But for consistency it's probs better to have matrix_options and linear_options for effects/stop too.

tests for now can probably just care about power_on.
We can add power_off as it's own separate option for sure, that'd be easy.

I just noticed that effects/stop can send an apply_theme and power_on is part of theme_options.


[wip] Improve subscribing / unsubscribing of Rx.Subscriptions #898

closes #898

Close in favour of #975


adds functions for async array traversal. adds CUSTOM pattern field f…

…or field formats. updates ember-concurrency to add support for waitForProperty, and updates typings.

Codecov Report

Merging #1133 into master will decrease coverage by 0.35%.
The diff coverage is 34.32%.

@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
- Coverage   32.38%   32.02%   -0.36%
==========================================
  Files         192      192
  Lines        4839     4893      +54
  Branches     1329     1343      +14
==========================================
  Hits         1567     1567
- Misses       3272     3326      +54

Impacted Files
Coverage Δ

...eb/app/components/dataset-compliance-rollup-row.ts
19.67% <ø> (ø)
:arrow_up:

wherehows-web/app/utils/validators/regexp.ts
100% <100%> (ø)
:arrow_up:

wherehows-web/app/utils/validators/urn.ts
86.66% <100%> (-2.99%)
:arrow_down:

wherehows-web/app/constants/datasets/compliance.ts
100% <100%> (ø)
:arrow_up:

...web/app/components/dataset-compliance-field-tag.ts
22.64% <20%> (-3.55%)
:arrow_down:

wherehows-web/app/components/dataset-compliance.ts
22.14% <5.26%> (-0.36%)
:arrow_down:

wherehows-web/app/constants/dataset-compliance.ts
78.31% <66.66%> (-0.91%)
:arrow_down:

...mponents/datasets/containers/dataset-compliance.ts
60.65% <66.66%> (-1.64%)
:arrow_down:

wherehows-web/app/utils/array.ts
91.3% <85.71%> (-8.7%)
:arrow_down:

wherehows-web/app/services/notifications.ts
35% <0%> (-28.75%)
:arrow_down:

... and 7 more

Continue to review full report at Codecov.

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

Coverage remained the same at 26.687% when pulling 933982bc0deea47dc3217ebf162983c3df95a64a on theseyi:async-iteration-and-custom-pattern into 720c2f5280a63029ddc5866a6c789b8421f85177 on linkedin:master.


[FSDE-1106][FSDE-1123] Added support of Server Side Pagination feature to Chi Vue Data Table component

Commit summary

[x] Added support of Server Side Pagination and an example of it
[x] Improved pagination, sorting, and row expansion event emission
[x] Added support of selected: true config for the rows and an example of automatic selection of nested rows
[x] Improved Select All to support multiple tables on the same page

@dani-cl-madrid @SergioQCL @aryaroudi @jllr @evansito
https://ctl.atlassian.net/browse/FSDE-1106
https://user-images.githubusercontent.com/25136131/124826814-9346f200-df75-11eb-8cab-989a2d6f0f71.mov
https://user-images.githubusercontent.com/25136131/125293279-8b939e80-e323-11eb-9390-ca0e7bc9434d.mov

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/1/. :x:

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/2/. :x:

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/3/. :x:

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/4/. :x:

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/5/. :x:

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/6/. :x:

The CI pipeline run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/7/. :white_check_mark:

You can check this PRs instance in https://e426fd9db9a9db74211d5cd2b8125d4e815dd9ec-chi-pr.enterprise.ctl.io

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/8/. :x:

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/9/. :x:

The CI pipeline run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/10/. :white_check_mark:

You can check this PRs instance in https://8a07920b6de5b2d96076d90e19aad2164b76d675-chi-pr.enterprise.ctl.io

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/11/. :x:

The CI pipeline run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/12/. :white_check_mark:

You can check this PRs instance in https://656d342aeb8468496f3c2defaf38e3379a9630f6-chi-pr.enterprise.ctl.io

The CI pipeline did not run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/13/. :x:

The CI pipeline run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/14/. :white_check_mark:

You can check this PRs instance in https://a09b98be5c1acbc3aa1937ec31daf93aaafdcf8e-chi-pr.enterprise.ctl.io

The CI pipeline run successfully in https://jenkinsprod.corp.intranet:8443/job/UX-CHI/job/Productive/job/Chi/job/PR-100/15/. :white_check_mark:

You can check this PRs instance in https://eeb3431ffd1e8bfec4546ba802ef78a9b85b3ee3-chi-pr.enterprise.ctl.io


Unwanted aggregation map_blocks

What happened:
While working on https://github.com/dask/dask/issues/8822 I tried to use drop_axis=True in map_blocks and ran into an issue where map_blocks aggregated the results from each block rather than concatenating them.
What you expected to happen:
The output should contain one output per block.
array([1., 2., 3.])

Minimal Complete Verifiable Example:
import numpy as np
import dask.array as da

array = dask.array.from_array([1,1,2,2,2,3,3,3,3,3], chunks=((2,3,5)))
array.map_blocks(np.mean, drop_axis=[0]).compute()
# 2.3

Ping @rjzamora for blockwise insight.

I'm not as familiar with dask.array as I'd like to be, but doesn't passing drop_axis=[0] to map_blocks mean you want to concatenate along the only axis you have?

Hmm I guess I thought it would mean "the output of my chunk-function will have a different dimensionality than the input" But I still expect one result per block.

@jsignell @rjzamora
The blockwise() documentation states:

Any index, like i missing from the output index is interpreted as a contraction ... the passed function should expect an iterable of blocks on any array that holds that index. To receive arrays concatenated along contracted dimensions instead pass concatenate=True.

So map_blocks() calls blockwise() without any output axis labels (due to drop_axis=[0]).  But blockwise() compares input and output axis labels and sees that the output  is missing the only axis label that the input has so it concatenates all chunks of the input into one large chunk which np.mean then reduces to a scalar.
As far as I can see, this behavior should be expected as this is the only way for the return value of map_blocks()/blockwise() to remain consistent with the expected output dimensionality, or meta data.  I mean, if the output dask array is expected to have a dimensionality of 0, then only a single value must be returned, therefore the dask array must have only one chunk containing that value.  The only way to enforce this is to concatenate all the input chunks into one chunk before passing it to the chunk-function.

I'm not sure I agree with the "expected result" of
array([1., 2., 3.])

To me, the fundamental contract of map_blocks is that the function should transform an array-like into an array-like. There might be changes to the representation (like sparsity or cupy or whatever), or the shape, but it should always be an arraylike, and  returning a scalar violates that contract. Automatically concatenating the resulting scalars into a new array would have some pretty surprising and confusing results that, to me, should be avoided.
To most users, I think we should be striving to make chunks an implementation detail. Clearly, the performance characteristics of a computation will depend on chunking, but I don't think the answer should. If my code got different answers depending on how I chose my chunked, I'd be very puzzled, and it would be pretty easy to hit that scenario if I simply forgot to specify which concatenation axis I want for np.mean. So with your example above
array = dask.array.from_array([1,1,2,2,2,3,3,3,3,3], chunks=((2,3,5)))
array.map_blocks(np.mean, drop_axis=[0]).compute()  # [1, 2, 3]

array = dask.array.from_array([1,1,2,2,2,3,3,3,3,3], chunks=((5,5)))
array.map_blocks(np.mean, drop_axis=[0]).compute()  # [1.6, 3]  Weird!

Now, users could still make cases that have that above behavior by ensuring that their reduction function keeps their axes (this gets your expected behavior):
array = dask.array.from_array([1,1,2,2,2,3,3,3,3,3], chunks=((2,3,5)))
array.map_blocks(np.mean, keepdims=True).compute()  # [1, 2, 3]

But I guess I'd be in favor of making people opt-in to chunking-dependent behavior with array-forcing workarounds like the above. So I guess I agree with @dcherian's suggestion in #8822 that a better error message if a scalar is returned is the right behavior.

Hmm yeah I can see how the behavior that I am describing in this issue might be unwanted and surprising. I guess I'm more familiar with the dataframe equivalent which does always return at least one value per partition and wraps scalars in Series objects if needed. But it is entirely possible that they are different enough that it's not worth trying to make their behavior internally consistent.
The name map_blocks to me implies a simple mapping, so the fact that it's possible to get a cross-block aggregation is what really confused me. I understand how it's possible given that map_blocks is backed by blockwise but I'm not sure it should be possible.
But this is probably orthogonal to @dcherian's original issue. I totally agree that an error message makes sense in that case. It can probably be raised at graph construction.

Here's an illustrative diagram that could be added to the docs to clarify the case of drop_axis != None:


Document task::spawn's support for blocking tasks.

Fixes #905.

Currently this is not true anymore.
I'm working on something to maybe bring it back though

Currently this is not true anymore.

Oh! Well, hmm. As I say in #905, once you've promised this behavior, taking it back would break all users, so it's only a feature to ship if you're willing to live with it permanently. If we've had to revert it once already, I hope that doesn't mean it's tricky.

Currently this is not true anymore.

@Keruspe Can you link to the issue?

Since this isn't currently planned, closing this; if in the future we re-add such support, we'll need to document it as well.


Hovering Effect

🚀 Feature
In the band page , I would like to add hovering effect of zoom out with box shadow
Motivation

/assign
lgmsoc participant

I would like to contribute on this issue under LGM SOC 21, Could you please assign this issue to me?

Assigned @anushka-srivastava22 , go ahead.


Microsoft.CodeAnalysis dependency conflict for LamarCompiler+ASP.NET Core MVC5

Microsoft.CodeAnalysis dependency conflict for LamarCompiler+ASP.NET Core MVC5 on 4.x
error NU1107: Version conflict detected for Microsoft.CodeAnalysis.Common. Install/reference Microsoft.CodeAnalysis.Common 3.7.0 directly to project Ticketing.Apps.JobProcessing to resolve this issue.

error NU1107:  Ticketing.Apps.JobProcessing -> Ticketing.Apps.Common -> Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation 5.0.0 -> Microsoft.CodeAnalysis.Razor 5.0.0 -> Microsoft.CodeAnalysis.Common (>= 3.7.0)

error NU1107:  Ticketing.Apps.JobProcessing -> Ticketing.Services -> Ticketing.Services.Common -> Marten 4.0.0-alpha.3 -> LamarCompiler 2.3.0 -> Microsoft.CodeAnalysis 3.6.0 -> Microsoft.CodeAnalysis.CSharp.Workspaces 3.6.0 -> Microsoft.CodeAnalysis.Common (= 3.6.0).

Realise this is a bit bleeding edge (as it was only released this week), but we've just rolled all of our dependencies up to 5.0 versions (.NET Core, EF Core, NpgSql) and I was surprised that Marten pins its NpgSql dependency to 4.x. Of course when running queries this results in MethodNotFoundException so its for a good reason!
Just wanted to log an issue to make sure you're aware this is a scenario that's broken right now.

Is this what you are looking for 😃

Ha, I am a moron, how did I not see that the first time I searched for it?
:-)
Thanks!
On Fri, 20 Nov 2020, 20:22 cocowalla<EMAIL_ADDRESS>wrote:

Is this https://github.com/JasperFx/marten/pull/1600 what you are
looking for 😃
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/JasperFx/marten/issues/1619#issuecomment-731389221,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAJRMVN6NIBWX3PNMSINUTSQ3FZTANCNFSM4T5GETQA
.

Just tried to upgrade to that alpha version - and actually still having issues so reopening the ticket (let me know if you want me to either adjust the title or open a new separate one instead).

error NU1107:  Ticketing.Apps.JobProcessing -> Ticketing.Apps.Common -> Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation 5.0.0 -> Microsoft.CodeAnalysis.Razor 5.0.0 -> Microsoft.CodeAnalysis.Common (>= 3.7.0)

error NU1107:  Ticketing.Apps.JobProcessing -> Ticketing.Services -> Ticketing.Services.Common -> Marten 4.0.0-alpha.3 -> LamarCompiler 2.3.0 -> Microsoft.CodeAnalysis 3.6.0 -> Microsoft.CodeAnalysis.CSharp.Workspaces 3.6.0 -> Microsoft.CodeAnalysis.Common (= 3.6.0). ```

Looks like a pretty standard conflict (I think LamarCompiler is part of Marten?) - though not sure if that pinned version is intentional or not. I'm going to try adding the explicit reference and will test if there are any unforseen issues (I assume since these are analysis packages thats unlikely though).

Anything else you need let me know!

@kieranbenton Let me check and revert on the issue which you have outlined. It is not exactly related to Npgsql5 support but rather some common dependency issue when combined with ASP.NET Core MVC.

@kieranbentonv <IP_ADDRESS>-alpha.4 is available in NuGet with updated version of LamarCompiler. Please get latest and confirm whether  the Microsoft.CodeAnalysis package resolves fine without any dependency conflict.

Fixes via https://github.com/JasperFx/marten/commit/5b76082ca8363a5df2e6c1cd6b423ebfcdd8c598

Looking good @mysticmind thank you!
Can I just check something? You've renamed the issue to "...when using with ASP.NET Core MVC on 4.x", what do you mean by 4.x? As far as I know I'm running all the latest .NET 5 bits now...

4.x is the current Marten version as a context. Bugs can reported on both 3.x and 4.x. I have added a tag now for tracking.

Got it - thanks! Think we're good now.

Also see this message with fsharp/dotnet 6 and marten 4.3.1
Using marten 5.0.0-alpha.6 the message is not logged


fix: Retry logic was never applied.

The implement of the Call object being stateful,
the retry logic was never triggered as an IllegalStateException was bypassing the normal flow.
Closes: #144

Thanks @olivier-hubaut !


permission denied

ADD ISSUE DESCRIPTION HERE
Version: 1.76.1
Commit: 5e805b79fcb6ba4c2d23712967df89a089da575b
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/<IP_ADDRESS> Safari/537.36 Edg/110.0.1587.69
Embedder: codespaces

/needsMoreInfo

Thanks for creating this issue! We figured it's missing some basic information or in some other way doesn't follow our issue reporting guidelines. Please take the time to review these and update the issue.
Happy Coding!

This issue has been closed automatically because it needs more information and has not had recent activity. See also our issue reporting guidelines.
Happy Coding!


renaming k to dnx

fix #9

Maybe we should rename to gulp-dnx? Not sure the aspnet work will last for long

Yes, correct. Good though.
It can be dnx only but we also have some web specific stuff like calling web command by default.
If we can remove those conventions, why not. What do u think?

I already removed the web command. no the dnxCommand is required

Ah, cool! Yes, gulp-dnx seems better than. Do you want to update it accordingly?

Yep. We'll do it today

:+1:

@tugberkugurlu Let me know when you review this. I changed the dnxCommand to be the first parameter of the function. It's all in the readme.

any news? :)

Damn, busy days :s on my phone now but will look at it and merge it in about two hours :smile: sorry for the delay :s

no problem. I want to fix the other problem too but I need this merged: https://github.com/npm/npm/issues/8359#issuecomment-106572853


debug mode collides with JSON Patch versioning

validatePatches is being called before operation objects are shifted from entire JSON Patch sequence, so it complains about patching non-existing nodes.
Probably it may not be an issue when we separate Puppet engine, from validation, and from JSON Patch queues, that we already thought about.

@tomalec is this issue still relevant? It looks like the validation has been moved to https://github.com/Starcounter-Jack/JSON-Patch/blob/master/src/json-patch-duplex.js#L622.

I'm not sure if it's still reproducible, but we still validate on apply https://github.com/Starcounter-Jack/JSON-Patch/blob/master/src/json-patch-duplex.js#L549
The question is how PuppetJS uses it with versioning.

I assume this is not a problem anymore or it would surface again. Archiving.


Deal with autodoc issues

The autodoc functionality that we're relying on for the Sphinx docs only somewhat works.

Many of the doc pages have bold type in the docstrings that probably results from a ** character somewhere, maybe a **kwargs or something, that's translated into formatting.
The docs won't build at all under the newest Sphinx 1.3.x, and as a result won't built on RTD.

We may have to look this over as part of a more general overhaul to the method of dealing with autodoc.

There's a deep problem in item 1 above, I think. It appears that the models currently aren't inheriting our custom docstrings at all for the documentation, they're just using something from scipy. Note, for example, that translate_args isn't showing up in either discrete or continuous docstrings.


Add new RP Microsoft.SecurityAndCompliance

MSFT employees can try out our new experience at OpenAPI Hub  - one location for using our validation tools and finding your workflow.

Changelog
Please ensure to add changelog with this PR by answering the following questions.

What's the purpose of the update?

[x] new service onboarding
[ ] new API version
[ ] update existing version for new feature
[ ] update existing version to fix swagger quality issue in s360
[ ] Other, please clarify

When you are targeting to deploy new service/feature to public regions? Please provide date, or month to public if date is not available yet.
When you expect to publish swagger? Please provide date, or month to public if date is not available yet.
If it's an update to existing version,  please select SDKs of specific language and CLIs that require refresh after swagger is published.

[ ] SDK of .NET (need service team to ensure code readiness)
[ ] SDK of Python
[ ] SDK of Java
[ ] SDK of Js
[ ] SDK of Go
[ ] PowerShell
[ ] CLI
[ ] Terraform
[ ] No, no need to refresh for updates in this PR

Contribution checklist:

[x] I commit to follow the Breaking Change Policy of "no breaking changes"
[x] I have reviewed the documentation for the workflow.
[x] Validation tools were run on swagger spec(s) and errors have all been fixed in this PR. How to fix?

If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist

[x] Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.

Adding new API(s)
Adding a new API version
Adding a new service

[x] Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.

[ ] If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.

Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.

[ ] Removing API(s) in stable version
[ ] Removing properties in stable version
[ ] Removing API version(s) in stable version
[ ] Updating API in stable version with Breaking Change Validation errors
[ ] Updating API(s) in preview over 1 year

Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.

@jianyexi , could you have a look at lintDiff runtime errors?

@raych1 , could you please help me with swagger lintdiff pipeline errors?

Left some comments, please review them, the specs are similar so if you update one item, make sure to fix the other specs in this PR as well.

@raych1 , could you please help me with swagger lintdiff pipeline errors?

@jianyexi , can you help with this?

@sonalverm  For LintDiff error, please check all the swagger files under "package-2021-01-11",  there is  a double defined API path but with different : $.paths["/subscriptions/subscriptionId/providers/Microsoft.SecurityAndCompliance/checkNameAvailability"].post.operationId

@sonalverm For LintDiff error, please check all the swagger files under "package-2021-01-11", there is a double defined API path but with different : $.paths["/subscriptions/subscriptionId/providers/Microsoft.SecurityAndCompliance/checkNameAvailability"].post.operationId

could you please explain how do I fix this?

@jianyxi , @ruoxuan could you please help me in resolving pipeline errors and merging this PR?

@jianyxi , @ruoxuan could you please help me in resolving pipeline errors and merging this PR?

@sonalverm For LintDiff error, please check all the swagger files under "package-2021-01-11", there is a double defined API path but with different : $.paths["/subscriptions/subscriptionId/providers/Microsoft.SecurityAndCompliance/checkNameAvailability"].post.operationId

could you please explain how do I fix this?

@sonalverm ,  please check all the swaggers referenced under tag of package-2021-01-11  in readme.md, they have duplicated path defined. It only allows unique path in one package.

@ArcturusZhang , can you have a look at GO SDK failures?

@jianyxi, @ruoxuan could you please help me resolve autorest exception?

/azp run

/azp run

/azp run

/azp run

/azp run

/azp run

@msyyc , can you check if this PR causes python sdk breaking change?

For python, the PR is ok

For python, the PR is ok


Add Muslim Names Feature Request

Hi Team,
I am working on a Project where I am interested in the Muslim names only like Muslim first name, last name etc.
It would be great if we could fake names based on the religion only.
        Faker faker = new Faker();

        String name = faker.name().muslim().fullName(); // Miss Ayman Khan
        String firstName = faker.name().muslim().firstName(); // Maryam
        String lastName = faker.name().muslim().lastName(); // Ibrahim

Have you looked into creating your own custom faker? This looks like a great usecase for that.
https://www.datafaker.net/documentation/custom-providers/

Hi @bodiam
The said custom-providers can be worked around for me, and I can make it work. However, I want to make a contribution to the library regarding the same.

No problem, PRs always welcome. Feel free to send one!


Mediator pipeline behavior for specific commands

Hi,
I have a project with multiple commands and queries. I need to intercept some specific commands using the Mediator pipeline.
I only see examples with generics, intercepting all the commands. Is there a way to capture only specific commands?
Using Net core 3.1, default DI
Thank you in advance

There are a few closed issues that discuss this already that  you could search for.  What you want is a constrained generic handler. The default dotnet core DI doesn’t support these. You’ll have to switch to a 3rd party DI like Lamar, or you could probably do simple pattern matching in the behavior.
if (request is SomeRequestType)
{
   ...
}

There are a few closed issues that discuss this already that you could search for. What you want is a constrained generic handler. The default dotnet core DI doesn’t support these. You’ll have to switch to a 3rd party DI like Lamar, or you could probably do simple pattern matching in the behavior.
if (request is SomeRequestType)
{
   ...
}

Thanks for your answer @lilasquared
I was seeking for solutions except for pattern matching. Probably I will come back to Action FIlters

Switching DI will give you what you want also.  Other DIs are far more mature than the built in one.

Feel free to upvote this PR: https://github.com/dotnet/runtime/pull/39540 it would fix this.