Resource quotas

Implement buffer pools.
No flow control pushback is implemented (yet).
Provides a C++ API, and some primitive cancellation and channel closing mechanisms.
Implements a benchmark so that we can track how well the mechanism works as pushback mechanisms are refined.
Based on #8008

This change is

I've gotten through all but 7 files, although those are some of the most complex ones.  In particular, I've only barely started looking at resource_quota.c, because I realized that I need a better understanding of the intended structure of the code and data in order to competently review it.  (I'm sure I could figure it out by staring at it long enough, but it seems more efficient to ask you for help, especially since more comments are needed anyway.)

Reviewed 110 of 117 files at r1.
Review status: 110 of 117 files reviewed at latest revision, 26 unresolved discussions, some commit checks failed.

include/grpc/grpc.h, line 419 at r1 (raw file):

/** Fetch a vtable for a grpc_channel_arg that points to a grpc_resource_quota
 */
GRPCAPI const grpc_arg_pointer_vtable *grpc_resource_quota_arg_vtable(void);

Why use a method to get the vtable, rather than just exporting the symbol for the vtable itself?

include/grpc/impl/codegen/grpc_types.h, line 206 at r1 (raw file):

/** If non-zero, a pointer to a buffer pool (use grpc_resource_quota_arg_vtable
   to fetch an appropriate pointer arg vtable */
#define GRPC_ARG_BUFFER_POOL "grpc.resource_quota"

Suggest calling this GRPC_ARG_RESOURCE_QUOTA for consistency.

src/core/lib/iomgr/resource_quota.c, line 47 at r1 (raw file):

int grpc_resource_quota_trace = 0;

typedef bool (*bpstate_func)(grpc_exec_ctx *exec_ctx,

What does "pbstate" mean?  Please document what this is for.

src/core/lib/iomgr/resource_quota.c, line 53 at r1 (raw file):

  grpc_resource_user *head;
  grpc_resource_user *tail;
} grpc_resource_user_list;

This is already defined in the .h file, so it shouldn't be needed here.

src/core/lib/iomgr/resource_quota.c, line 55 at r1 (raw file):

} grpc_resource_user_list;

struct grpc_resource_quota {

Please document all fields.

src/core/lib/iomgr/resource_quota.c, line 67 at r1 (raw file):

  grpc_closure bpreclaimation_done_closure;

  grpc_resource_user *roots[GRPC_BULIST_COUNT];

I'm having trouble understanding the relationship between grpc_resource_quota and grpc_resource_user.  It seems that each one contains pointers to the other, but it's not clear how the ownership works.

src/core/lib/iomgr/resource_quota.c, line 82 at r1 (raw file):

  if (*root == NULL) {
    *root = resource_user;
    resource_user->links[list].next = resource_user->links[list].prev =

So this is a circular linked list?  Why do that instead of having the list terminated with a NULL pointer?

src/core/lib/iomgr/resource_quota.h, line 35 at r1 (raw file):

#ifndef GRPC_CORE_LIB_IOMGR_RESOURCE_QUOTA_H
#define GRPC_CORE_LIB_IOMGR_RESOURCE_QUOTA_H

This file has a ton of declarations, but not one comment.  Please add comments explaining what each object and non-obvious method is.  It would also be really helpful to have a block comment at the top explaining the concepts and how they fit together.

src/core/lib/iomgr/resource_quota.h, line 54 at r1 (raw file):

GRPC_BULIST_AWAITING_ALLOCATION,
GRPC_BULIST_NON_EMPTY_FREE_POOL,
GRPC_BULIST_RECLAIMER_BENIGN,
GRPC_BULIST_RECLAIMER_DESTRUCTIVE,

Please add comments indicating what each of these is for.

src/core/lib/iomgr/resource_quota.h, line 56 at r1 (raw file):

  GRPC_BULIST_RECLAIMER_DESTRUCTIVE,
  GRPC_BULIST_COUNT
} grpc_bulist;

What does "bulist" mean?  This needs a comment.

src/core/lib/iomgr/resource_quota.h, line 58 at r1 (raw file):

} grpc_bulist;

typedef struct grpc_resource_user grpc_resource_user;

When I first saw this name, I thought "user" meant an individual (i.e., a login).  It took me a little while to realize that it actually meant "consumer of resources".  Perhaps it would be better to use the term "pool" here?

src/core/lib/iomgr/resource_quota.h, line 77 at r1 (raw file):

int64_t allocated;
int64_t free_pool;

Please add comments explaining what these fields are.

src/core/lib/iomgr/resource_quota.h, line 83 at r1 (raw file):

grpc_closure *reclaimers[2];
grpc_closure post_reclaimer_closure[2];

Why are these arrays?  What do the two elements represent?  Please add comments.

src/core/lib/iomgr/resource_quota.h, line 102 at r1 (raw file):

void grpc_resource_user_alloc(grpc_exec_ctx *exec_ctx,
grpc_resource_user *resource_user, size_t size,
grpc_closure *optional_on_done);
void grpc_resource_user_free(grpc_exec_ctx *exec_ctx,
grpc_resource_user *resource_user, size_t size);
void grpc_resource_user_post_reclaimer(grpc_exec_ctx *exec_ctx,
grpc_resource_user *resource_user,
bool destructive, grpc_closure *closure);
void grpc_resource_user_finish_reclaimation(grpc_exec_ctx *exec_ctx,
grpc_resource_user *resource_user);

These functions need explanatory comments.

src/core/lib/iomgr/resource_quota.h, line 110 at r1 (raw file):

                                       grpc_resource_user *resource_user,
                                       bool destructive, grpc_closure *closure);
void grpc_resource_user_finish_reclaimation(grpc_exec_ctx *exec_ctx,

s/reclaimation/reclamation/

src/core/lib/iomgr/resource_quota.h, line 113 at r1 (raw file):

                                            grpc_resource_user *resource_user);

typedef struct grpc_resource_user_slice_allocator {

Please document the fields of this struct.

src/core/lib/iomgr/resource_quota.h, line 126 at r1 (raw file):

    grpc_resource_user *resource_user, grpc_iomgr_cb_func cb, void *p);

void grpc_resource_user_alloc_slices(

Please document what this function does.

src/core/lib/iomgr/tcp_client.h, line 42 at r1 (raw file):

#include "src/core/lib/iomgr/sockaddr.h"

#define GRPC_ARG_TCP_READ_CHUNK_SIZE "grpc.experimental.tcp_read_chunk_size"

Please add a comment explaining what this indicates and how it is used.

src/core/lib/surface/call.c, line 1518 at r1 (raw file):

              GRPC_MDSTR_REF(call->send_extra_metadata[1].md->value));
        }
        if (op->data.send_status_from_server.status != GRPC_STATUS_OK) {

Out of curiosity, how is this related to the rest of this PR?  I assume you encountered this during testing?  What exactly was the symptom of this?

src/core/lib/surface/server.c, line 845 at r1 (raw file):

  if (error != GRPC_ERROR_NONE) {
    got_initial_metadata(exec_ctx, elem, error);
    GRPC_ERROR_UNREF(error);

Similar curiosity question here: How did you discover this bug while working on the resource quota change?

test/core/end2end/gen_build_yaml.py, line 93 at r1 (raw file):

    'binary_metadata': default_test_options,
    'resource_quota_server': default_test_options._replace(large_writes=True,
                                                        proxyable=False),

Nit: Please fix indentation.

test/core/end2end/tests/max_message_length.c, line 405 at r1 (raw file):

  GPR_ASSERT(0 == strcmp(call_details.method, "/service/method"));
  GPR_ASSERT(0 == strcmp(call_details.host, "foo.test.google.fr:1234"));
  GPR_ASSERT(was_cancelled == 0);

Why this change?  This should certainly be true when the limit is exceeded on the client side, and perhaps on the server side too (although that can probably be argued).

test/core/end2end/tests/network_status_change.c, line 216 at r1 (raw file):

  GPR_ASSERT(0 == strcmp(call_details.method, "/foo"));
  GPR_ASSERT(0 == strcmp(call_details.host, "foo.test.google.fr"));
  GPR_ASSERT(was_cancelled == 0);

What's the reason for this change?

test/core/util/mock_endpoint.c, line 36 at r1 (raw file):

#include "test/core/util/mock_endpoint.h"

#include <inttypes.h>

Why was this added?  I don't see any non-built-in integer types being added.

test/core/util/passthru_endpoint.c, line 36 at r1 (raw file):

#include "test/core/util/passthru_endpoint.h"

#include <inttypes.h>

Why was this include added?  I don't see any integer types being added.

test/cpp/end2end/end2end_test.cc, line 1489 at r1 (raw file):

      : server_resource_quota_("server_resource_quota") {}

  virtual void ConfigureServerBuilder(ServerBuilder* builder) GRPC_OVERRIDE {

Can we use "override" instead of "virtual", or is that too new of a C++ feature?

Comments from Reviewable

Review status: 108 of 117 files reviewed at latest revision, 26 unresolved discussions, some commit checks failed.

include/grpc/grpc.h, line 419 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Why use a method to get the vtable, rather than just exporting the symbol for the vtable itself?

Windows.
We have build situations where we need to do the equivalent of dlopen(), dlsym() for every symbol in the public API. When we're in that world, the dlsym variant doesn't have a facility to get at variables, and so it's necessary that we export a function.

include/grpc/impl/codegen/grpc_types.h, line 206 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Suggest calling this `GRPC_ARG_RESOURCE_QUOTA` for consistency.

Search/replace oversight. Fixed.

src/core/lib/iomgr/resource_quota.c, line 47 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> What does "pbstate" mean?  Please document what this is for.

Old code, deleted

src/core/lib/iomgr/resource_quota.c, line 53 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> This is already defined in the .h file, so it shouldn't be needed here.

Old code, deleted

src/core/lib/iomgr/resource_quota.c, line 55 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Please document all fields.

Done.

src/core/lib/iomgr/resource_quota.c, line 67 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> I'm having trouble understanding the relationship between `grpc_resource_quota` and `grpc_resource_user`.  It seems that each one contains pointers to the other, but it's not clear how the ownership works.

Done.

src/core/lib/iomgr/resource_quota.c, line 82 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> So this is a circular linked list?  Why do that instead of having the list terminated with a NULL pointer?

Saves a few ifs... no big other reason

src/core/lib/iomgr/resource_quota.h, line 35 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> This file has a ton of declarations, but not one comment.  Please add comments explaining what each object and non-obvious method is.  It would also be really helpful to have a block comment at the top explaining the concepts and how they fit together.

Done.

src/core/lib/iomgr/resource_quota.h, line 54 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >   GRPC_BULIST_AWAITING_ALLOCATION,
> >   GRPC_BULIST_NON_EMPTY_FREE_POOL,
> >   GRPC_BULIST_RECLAIMER_BENIGN,
> >   GRPC_BULIST_RECLAIMER_DESTRUCTIVE,
>
> Please add comments indicating what each of these is for.

Done.

src/core/lib/iomgr/resource_quota.h, line 56 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> What does "bulist" mean?  This needs a comment.

Done.

src/core/lib/iomgr/resource_quota.h, line 58 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> When I first saw this name, I thought "user" meant an individual (i.e., a login).  It took me a little while to realize that it actually meant "consumer of resources".  Perhaps it would be better to use the term "pool" here?

Done.

src/core/lib/iomgr/resource_quota.h, line 77 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >   int64_t allocated;
> >   int64_t free_pool;
>
> Please add comments explaining what these fields are.

Done.

src/core/lib/iomgr/resource_quota.h, line 83 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >   grpc_closure *reclaimers[2];
> >   grpc_closure post_reclaimer_closure[2];
>
> Why are these arrays?  What do the two elements represent?  Please add comments.

Done.

src/core/lib/iomgr/resource_quota.h, line 102 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> > void grpc_resource_user_alloc(grpc_exec_ctx *exec_ctx,
> >                               grpc_resource_user *resource_user, size_t size,
> >                               grpc_closure *optional_on_done);
> > void grpc_resource_user_free(grpc_exec_ctx *exec_ctx,
> >                              grpc_resource_user *resource_user, size_t size);
> > void grpc_resource_user_post_reclaimer(grpc_exec_ctx *exec_ctx,
> >                                        grpc_resource_user *resource_user,
> >                                        bool destructive, grpc_closure *closure);
> > void grpc_resource_user_finish_reclaimation(grpc_exec_ctx *exec_ctx,
> >                                             grpc_resource_user *resource_user);
>
> These functions need explanatory comments.

Done.

src/core/lib/iomgr/resource_quota.h, line 110 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> s/reclaimation/reclamation/

Done.

src/core/lib/iomgr/resource_quota.h, line 113 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Please document the fields of this struct.

Done.

src/core/lib/iomgr/resource_quota.h, line 126 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Please document what this function does.

Done.

src/core/lib/iomgr/tcp_client.h, line 42 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Please add a comment explaining what this indicates and how it is used.

Done.

src/core/lib/surface/call.c, line 1518 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Out of curiosity, how is this related to the rest of this PR?  I assume you encountered this during testing?  What exactly was the symptom of this?

The symptom was getting an ok status where we should have seen a cancellation on the server side... a bunch of tests were buggy.

src/core/lib/surface/server.c, line 845 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Similar curiosity question here: How did you discover this bug while working on the resource quota change?

I guess we started executing this code path.... I don't recall how (but in putting this together we do hit a lot more paths that were previously of theoretical interest).

test/core/end2end/gen_build_yaml.py, line 93 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Nit: Please fix indentation.

Done.

test/core/end2end/tests/max_message_length.c, line 405 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Why this change?  This should certainly be true when the limit is exceeded on the client side, and perhaps on the server side too (although that can probably be argued).

It should be cancelled when exceeded on the server side... so we should probably check == 1... but it's an argument, and I'd argue that there's not much point constraining it.

test/core/end2end/tests/network_status_change.c, line 216 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> What's the reason for this change?

It's wrong. If we forcefully close a connection, we shouldn't be forcing a non-cancelled status.

test/core/util/mock_endpoint.c, line 36 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Why was this added?  I don't see any non-built-in integer types being added.

PRIxPTR

test/core/util/passthru_endpoint.c, line 36 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Why was this include added?  I don't see any integer types being added.

PRIxPTR

test/cpp/end2end/end2end_test.cc, line 1489 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Can we use "override" instead of "virtual", or is that too new of a C++ feature?

GRPC_OVERRIDE means that for C++11 compilers.

Comments from Reviewable

Okay, I'm finally done reviewing this.  Overall, the code looks pretty good.  Hopefully, my comments are useful.

Reviewed 2 of 117 files at r1, 2 of 2 files at r2, 14 of 14 files at r3, 23 of 23 files at r4.
Review status: all files reviewed at latest revision, 39 unresolved discussions.

include/grpc/grpc.h, line 419 at r1 (raw file):
Previously, ctiller (Craig Tiller) wrote…
> Windows.
>
> We have build situations where we need to do the equivalent of dlopen(), dlsym() for every symbol in the public API. When we're in that world, the dlsym variant doesn't have a facility to get at variables, and so it's necessary that we export a function.

Interesting.  I assume that this is only necessary in the public C-core API, not internal calls?  For example, I know that we have filter code where the .h file declares the filter struct instead of a function to get it.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2111 at r3 (raw file):

/*******************************************************************************
 * BUFFER POOLS

RESOURCE QUOTAS

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2116 at r3 (raw file):

static void post_benign_reclaimer(grpc_exec_ctx *exec_ctx,
                                  grpc_chttp2_transport *t) {
  if (!t->benign_reclaimer_registered) {

Do we need to hold a mutex to access and modify this field?  Or is this handled by combiner locks?

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2127 at r3 (raw file):

static void post_destructive_reclaimer(grpc_exec_ctx *exec_ctx,
                                       grpc_chttp2_transport *t) {
  if (!t->destructive_reclaimer_registered) {

Same question here.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2159 at r3 (raw file):

              t->peer_string);
    }
    send_goaway(exec_ctx, t, GRPC_CHTTP2_ENHANCE_YOUR_CALM,

It initially confused me as to why we are sending a GOAWAY from the benign reclaimer, but then I noticed that this only happens when there are no open streams.  It might be worth a comment here indicating that the goal is to close idle connections.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2190 at r3 (raw file):

if (n > 1) {
  post_destructive_reclaimer(exec_ctx, t);
}

I assume that the idea here is to issue a separate reclaimer callback for each stream, so that we stop when we've reclaimed enough memory?  If so, it's probably worth a comment.

src/core/lib/iomgr/resource_quota.c, line 47 at r3 (raw file):

int grpc_resource_quota_trace = 0;

struct grpc_resource_quota {

Do we not need a mutex for any of the data members here?  If not, please add a comment explaining why not.
(I suspect this is because all of the callbacks that modify data members will be run via the combiner lock, but I would like to be sure my understanding is correct.)

src/core/lib/iomgr/resource_quota.c, line 104 at r3 (raw file):

resource_user->links[list].next = (*root)->links[list].next;
resource_user->links[list].prev = *root;

If this is inserting at the head of the list, shouldn't the old root node go after the new node, not before it?  In effect, it seems like this moves the old root node to the end of the list.
For example, let's say that we start with 3 nodes, A, B, and C, where A is the root.  The list will look like A -> B -> C -> A.  If we then insert new node N at the head, we will wind up with N as the new root node, and the list will look like N -> B -> C -> A -> N.  But it seems like what should actually happen in this case is N -> A -> B -> C -> N.
I think these two lines should be:
resource_user->links[list].next = *root;
resource_user->links[list].prev = (*root)->links[list].prev;

src/core/lib/iomgr/resource_quota.c, line 152 at r3 (raw file):

/*******************************************************************************
 * buffer pool state machine

s/buffer pool/resource quota/

src/core/lib/iomgr/resource_quota.c, line 162 at r3 (raw file):

                       grpc_resource_quota *resource_quota, bool destructive);

static void rq_step(grpc_exec_ctx *exec_ctx, void *bp, grpc_error *error) {

s/bp/rq/ (or perhaps just call it "arg"?)

src/core/lib/iomgr/resource_quota.c, line 197 at r3 (raw file):

      resource_quota->free_pool -= amt;
      if (grpc_resource_quota_trace) {
        gpr_log(GPR_DEBUG, "BP %s %s: grant alloc %" PRId64

s/BP/RESOURCE QUOTA/
Same thing for all trace logging in this file.

src/core/lib/iomgr/resource_quota.c, line 220 at r3 (raw file):

/* returns true if any memory could be reclaimed from buffers */
static bool rq_scavenge(grpc_exec_ctx *exec_ctx,

The term "scavenge" doesn't map to any of the existing terms used in comments or member names.  Suggest calling this something like rq_reclaim_interally() or perhaps rq_reclaim_from_freepool().

src/core/lib/iomgr/resource_quota.c, line 317 at r3 (raw file):

/*******************************************************************************
 * grpc_resource_quota internal implementation

Most of the functions in this section seem to have more to do with the user than the quota.  Maybe split them into two sections?

src/core/lib/iomgr/resource_quota.c, line 320 at r3 (raw file):

 */

static void ru_allocate(grpc_exec_ctx *exec_ctx, void *bu, grpc_error *error) {

s/bu/ru/ (or perhaps just call this "arg"?)
Same thing for all of these ru_* functions.

src/core/lib/iomgr/resource_quota.c, line 326 at r3 (raw file):

if (rulist_empty(resource_user->resource_quota,
GRPC_RULIST_AWAITING_ALLOCATION)) {
rq_step_sched(exec_ctx, resource_user->resource_quota);
}
rulist_add_tail(resource_user, GRPC_RULIST_AWAITING_ALLOCATION);

Don't we need to update the list before we schedule the reclamation/allocation step?  What happens if the step starts in another thread before the element is added to the list?
Same question for all of these ru_* functions.

src/core/lib/iomgr/resource_quota.c, line 390 at r3 (raw file):

}

static void ru_allocated_slices(grpc_exec_ctx *exec_ctx, void *ts,

What does ts stand for?  Perhaps just call this arg?

src/core/lib/iomgr/resource_quota.c, line 414 at r3 (raw file):

  a->resource_quota->size += delta;
  a->resource_quota->free_pool += delta;
  if (delta < 0 && a->resource_quota->free_pool < 0) {

Out of curiosity, is there a reason to only schedule a step if the delta is negative?  It seems like it wouldn't hurt anything to do it if freepool is negative, even if the delta was positive.

src/core/lib/iomgr/resource_quota.c, line 425 at r3 (raw file):

}

static void rq_reclamation_done(grpc_exec_ctx *exec_ctx, void *bp,

s/bp/rq/ (or just call it "args")

src/core/lib/iomgr/resource_quota.c, line 470 at r3 (raw file):

void grpc_resource_quota_unref(grpc_resource_quota *resource_quota) {
  grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;

Why create our own exec_ctx here, rather than have the caller pass it in?
Oh, hmm, looks like this is because this needs to be called from the channel_args vtable.  How do we know it's safe to create our own exec_ctx here?

src/core/lib/iomgr/resource_quota.c, line 481 at r3 (raw file):

}

void grpc_resource_quota_ref(grpc_resource_quota *resource_quota) {

Why not just have the public method return the resource_quota object, so there's no need for a separate internal and external version?

src/core/lib/iomgr/resource_quota.c, line 487 at r3 (raw file):

void grpc_resource_quota_resize(grpc_resource_quota *resource_quota,
                                size_t size) {
  grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;

How do we know it's safe to create our own exec_ctx here?  Please add a comment.

src/core/lib/iomgr/resource_quota.h, line 113 at r1 (raw file):
Previously, ctiller (Craig Tiller) wrote…
> Done.

Doesn't look like the individual fields have been documented.

src/core/lib/iomgr/resource_quota.h, line 47 at r3 (raw file):

    A grpc_resource_quota represents the pooled resources, and
    grpc_resource_user instances attach to the quota and consume those

Thanks for this great comment -- this makes a lot of things clearer.
Now that I understand the relationship between grpc_resource_quota and grpc_resource_user, I'm still not that enthusiastic about the term "user" here.  Alternative suggestions: "consumer", "allocator", or "usage".

src/core/lib/iomgr/resource_quota.h, line 63 at r3 (raw file):

These reclamations are tried in priority order, and only one reclamation
is outstanding for a quota at any given time (meaning that if a destructive
reclamation makes progress, we may follow up with a benign reclamation).

Isn't this example backwards?  If the reclamations are tried in priority order, wouldn't benign be tried before destructive?

src/core/lib/iomgr/resource_quota.h, line 85 at r3 (raw file):

typedef enum {
  /* Resource users that are waiting for an allocation */
  GRPC_RULIST_AWAITING_ALLOCATION,

I generally prefer to err on the side of having names that are too long rather than using abbreviations that make the code less clear to readers.  So I'd personally prefer to see this called GRPC_RESOURCE_USER_LIST_AWAITING_ALLOCATION, or perhaps GRPC_RES_USER_LIST_AWAITING_ALLOCATION (and same for the rest of these enum values).
Obviously, this is a personal taste issue, so please feel free to push back.

src/core/lib/iomgr/resource_quota.h, line 109 at r3 (raw file):

  grpc_resource_quota *resource_quota;

  /* Closure to schedule an allocation onder the resource quota combiner lock */

s/onder/under/

src/core/lib/iomgr/resource_quota.h, line 117 at r3 (raw file):

/* Canary object to detect leaked resource users with ASAN */
void *asan_canary;

How exactly does this work?

src/core/lib/iomgr/resource_quota.h, line 121 at r3 (raw file):

  gpr_mu mu;
  /* Total allocated memory outstanding by this resource user;

In bytes, I assume?  Please indicate units in the comment.

src/core/lib/iomgr/resource_quota.h, line 124 at r3 (raw file):

     always positive */
  int64_t allocated;
  /* The amount of memory this user has cached for its own use: to avoid quota

Please indicate units here too.

src/core/lib/iomgr/resource_quota.h, line 125 at r3 (raw file):

  int64_t allocated;
  /* The amount of memory this user has cached for its own use: to avoid quota
     contention, each resource user can keep some memory aside from the quota,

If I understand the intent here correctly, then I think this wording is misleading.  The phrase "aside from the quota" implies that the resource user is getting resources from something other than the quota object.  Suggested alternative phrasing: "can keep some memory in addition to what it is immediately using (e.g., for caching)".

src/core/lib/iomgr/resource_quota.h, line 152 at r3 (raw file):

/* User supplied closure to call once the user has finished shutting down AND
all outstanding allocations have been freed */
gpr_atm on_done_destroy_closure;

If this is a closure, how come the type is gpr_atm?  I'm guessing that this is actually interpreted as a pointer to a closure.  If so, please note that in the comment.

src/core/lib/iomgr/resource_quota.h, line 170 at r3 (raw file):

                                grpc_resource_user *resource_user);

/* Allocate from the resource user (and it's quota).

Grammar nit: s/it's/its/

src/core/lib/iomgr/resource_quota.h, line 205 at r3 (raw file):

void grpc_resource_user_slice_allocator_init(
    grpc_resource_user_slice_allocator *slice_allocator,
    grpc_resource_user *resource_user, grpc_iomgr_cb_func cb, void *p);

Please document the cb and p arguments.

src/core/lib/iomgr/tcp_posix.c, line 574 at r3 (raw file):

void grpc_tcp_destroy_and_release_fd(grpc_exec_ctx *exec_ctx, grpc_endpoint *ep,
                                     int *fd, grpc_closure *done) {
  grpc_network_status_unregister_endpoint(ep);

This seems unrelated to the rest of the change.  Is there some connection I'm not seeing, or is this just something unrelated that you noticed while working on this code?

test/core/end2end/tests/buffer_pool_server.c, line 1 at r3 (raw file):

/*

This file looks almost identical to resource_quota_server.c.  I'm guessing that you renamed this but forgot to remove the old version?

test/core/end2end/tests/resource_quota_server.c, line 343 at r3 (raw file):

GPR_ASSERT(cancelled_calls_on_client >= cancelled_calls_on_server);
GPR_ASSERT(cancelled_calls_on_server >= 0.9 * cancelled_calls_on_client);

There's probably something I'm missing here, but why won't these numbers exactly add up?  And where does the 0.9 factor come from?

test/core/iomgr/resource_quota_test.c, line 99 at r3 (raw file):

static void test_resize_then_destroy(void) {
  gpr_log(GPR_INFO, "** test_resize_then_destroy **");
  grpc_resource_quota *p =

Suggest using "q" instead of "p" in all of these tests.

test/core/iomgr/resource_quota_test.c, line 553 at r3 (raw file):

}

static void test_pools_merged_on_resource_user_deletion(void) {

I don't understand what "pools merged" means.  It looks like the test is creating and then destroying users, one at a time.  What's being merged here?

test/cpp/end2end/end2end_test.cc, line 1489 at r1 (raw file):
Previously, ctiller (Craig Tiller) wrote…
> GRPC_OVERRIDE means that for C++11 compilers.

Oh, I see -- but there's no way to drop the "virtual", because in pre-C++11 compilers, GRPC_OVERRIDE expands to an empty string?  That's too bad.

Comments from Reviewable

Review status: all files reviewed at latest revision, 37 unresolved discussions, some commit checks failed.

include/grpc/grpc.h, line 419 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Interesting.  I assume that this is only necessary in the public C-core API, not internal calls?  For example, I know that we have filter code where the .h file declares the filter struct instead of a function to get it.

Yeah, just for the public API surface. It's never required for internals.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2111 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> RESOURCE QUOTAS

Done.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2116 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Do we need to hold a mutex to access and modify this field?  Or is this handled by combiner locks?

The latter.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2127 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Same question here.

The latter.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2159 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> It initially confused me as to why we are sending a GOAWAY from the benign reclaimer, but then I noticed that this only happens when there are no open streams.  It might be worth a comment here indicating that the goal is to close idle connections.

Done.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2190 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >     if (n > 1) {
> >       post_destructive_reclaimer(exec_ctx, t);
> >     }
>
> I assume that the idea here is to issue a separate reclaimer callback for each stream, so that we stop when we've reclaimed enough memory?  If so, it's probably worth a comment.

Done.

src/core/lib/iomgr/resource_quota.c, line 47 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Do we not need a mutex for any of the data members here?  If not, please add a comment explaining why not.
>
> (I suspect this is because all of the callbacks that modify data members will be run via the combiner lock, but I would like to be sure my understanding is correct.)

Done.

src/core/lib/iomgr/resource_quota.c, line 104 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >     resource_user->links[list].next = (*root)->links[list].next;
> >     resource_user->links[list].prev = *root;
>
> If this is inserting at the head of the list, shouldn't the old root node go after the new node, not before it?  In effect, it seems like this moves the old root node to the end of the list.
>
> For example, let's say that we start with 3 nodes, A, B, and C, where A is the root.  The list will look like A -> B -> C -> A.  If we then insert new node N at the head, we will wind up with N as the new root node, and the list will look like N -> B -> C -> A -> N.  But it seems like what should actually happen in this case is N -> A -> B -> C -> N.
>
> I think these two lines should be:
>
> ```
> resource_user->links[list].next = *root;
> resource_user->links[list].prev = (*root)->links[list].prev;
> ```

To and fro a dozen times, and I agree.
Swapped implementations of add_head, add_tail.
Renamed pop() to pop_tail() for clarity.

src/core/lib/iomgr/resource_quota.c, line 152 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> s/buffer pool/resource quota/

Done.

src/core/lib/iomgr/resource_quota.c, line 162 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> s/bp/rq/ (or perhaps just call it "arg"?)

Done.

src/core/lib/iomgr/resource_quota.c, line 197 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> s/BP/RESOURCE QUOTA/
>
> Same thing for all trace logging in this file.

Done.

src/core/lib/iomgr/resource_quota.c, line 220 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> The term "scavenge" doesn't map to any of the existing terms used in comments or member names.  Suggest calling this something like `rq_reclaim_interally()` or perhaps `rq_reclaim_from_freepool()`.

Done.

src/core/lib/iomgr/resource_quota.c, line 317 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Most of the functions in this section seem to have more to do with the user than the quota.  Maybe split them into two sections?

Done.

src/core/lib/iomgr/resource_quota.c, line 320 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> s/bu/ru/ (or perhaps just call this "arg"?)
>
> Same thing for all of these `ru_*` functions.

Done.

src/core/lib/iomgr/resource_quota.c, line 326 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >   if (rulist_empty(resource_user->resource_quota,
> >                    GRPC_RULIST_AWAITING_ALLOCATION)) {
> >     rq_step_sched(exec_ctx, resource_user->resource_quota);
> >   }
> >   rulist_add_tail(resource_user, GRPC_RULIST_AWAITING_ALLOCATION);
>
> Don't we need to update the list before we schedule the reclamation/allocation step?  What happens if the step starts in another thread before the element is added to the list?
>
> Same question for all of these `ru_*` functions.

It's all done under a combiner lock, so this is safe.

src/core/lib/iomgr/resource_quota.c, line 390 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> What does `ts` stand for?  Perhaps just call this `arg`?

Done.

src/core/lib/iomgr/resource_quota.c, line 414 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Out of curiosity, is there a reason to only schedule a step if the delta is negative?  It seems like it wouldn't hurt anything to do it if freepool is negative, even if the delta was positive.

I think I was being overly conservative. That said, resizes shouldn't be frequent, so I'm going to go with a much simpler 'always schedule a step' here to avoid potential bugs.

src/core/lib/iomgr/resource_quota.c, line 425 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> s/bp/rq/ (or just call it "args")

Done.

src/core/lib/iomgr/resource_quota.c, line 470 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Why create our own exec_ctx here, rather than have the caller pass it in?
>
> Oh, hmm, looks like this is because this needs to be called from the channel_args vtable.  How do we know it's safe to create our own exec_ctx here?

This is a public API and so needs to create one... will add a comment here.

src/core/lib/iomgr/resource_quota.c, line 481 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Why not just have the public method return the resource_quota object, so there's no need for a separate internal and external version?

Mostly consistency with current public API's... I can be convinced otherwise.

src/core/lib/iomgr/resource_quota.c, line 487 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> How do we know it's safe to create our own exec_ctx here?  Please add a comment.

Done.

src/core/lib/iomgr/resource_quota.h, line 113 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Doesn't look like the individual fields have been documented.

Misclick. Done.

src/core/lib/iomgr/resource_quota.h, line 47 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Thanks for this great comment -- this makes a lot of things clearer.
>
> Now that I understand the relationship between grpc_resource_quota and grpc_resource_user, I'm still not that enthusiastic about the term "user" here.  Alternative suggestions: "consumer", "allocator", or "usage".

consumer, allocator: don't love them because it suggests a sink, and this is actually a collaborator
usage: mmmh, seems wrong in a way that I'm not sure I can convey right now

src/core/lib/iomgr/resource_quota.h, line 63 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >     These reclamations are tried in priority order, and only one reclamation
> >     is outstanding for a quota at any given time (meaning that if a destructive
> >     reclamation makes progress, we may follow up with a benign reclamation).
>
> Isn't this example backwards?  If the reclamations are tried in priority order, wouldn't benign be tried before destructive?

Benign will be tried before destructive. If we get to doing destructive steps, we'll only do so until benign (or internal) steps are available.

src/core/lib/iomgr/resource_quota.h, line 85 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> I generally prefer to err on the side of having names that are too long rather than using abbreviations that make the code less clear to readers.  So I'd personally prefer to see this called `GRPC_RESOURCE_USER_LIST_AWAITING_ALLOCATION`, or perhaps `GRPC_RES_USER_LIST_AWAITING_ALLOCATION` (and same for the rest of these enum values).
>
> Obviously, this is a personal taste issue, so please feel free to push back.

There's a balance here that's tricky: since the first part of the name is just namespacing, and the second part is semantically interesting, I typically try to keep the namespace part down to one identifier (+GRPC_), in order to be able to scan for the semantic information quickly.

src/core/lib/iomgr/resource_quota.h, line 109 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> s/onder/under/

No. I meant onder. :)
Fixed.

src/core/lib/iomgr/resource_quota.h, line 117 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >   /* Canary object to detect leaked resource users with ASAN */
> >   void *asan_canary;
>
> How exactly does this work?

We call malloc(), and ASAN ensures that there's a matching free(). If there's not, it fails the test.

src/core/lib/iomgr/resource_quota.h, line 121 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> In bytes, I assume?  Please indicate units in the comment.

Done.

src/core/lib/iomgr/resource_quota.h, line 124 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Please indicate units here too.

Done.

src/core/lib/iomgr/resource_quota.h, line 125 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> If I understand the intent here correctly, then I think this wording is misleading.  The phrase "aside from the quota" implies that the resource user is getting resources from something other than the quota object.  Suggested alternative phrasing: "can keep some memory in addition to what it is immediately using (e.g., for caching)".

Done.

src/core/lib/iomgr/resource_quota.h, line 152 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >   /* User supplied closure to call once the user has finished shutting down AND
> >      all outstanding allocations have been freed */
> >   gpr_atm on_done_destroy_closure;
>
> If this is a closure, how come the type is `gpr_atm`?  I'm guessing that this is actually interpreted as a pointer to a closure.  If so, please note that in the comment.

Done.

src/core/lib/iomgr/resource_quota.h, line 170 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Grammar nit: s/it's/its/

Done.

src/core/lib/iomgr/resource_quota.h, line 205 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Please document the `cb` and `p` arguments.

Done.

src/core/lib/iomgr/tcp_posix.c, line 574 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> This seems unrelated to the rest of the change.  Is there some connection I'm not seeing, or is this just something unrelated that you noticed while working on this code?

Not doing so triggers some memory leaks (and/or corruption) that gets exposed with this code.

test/core/end2end/tests/buffer_pool_server.c, line 1 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> This file looks almost identical to resource_quota_server.c.  I'm guessing that you renamed this but forgot to remove the old version?

Done.

test/core/end2end/tests/resource_quota_server.c, line 343 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >   GPR_ASSERT(cancelled_calls_on_client >= cancelled_calls_on_server);
> >   GPR_ASSERT(cancelled_calls_on_server >= 0.9 * cancelled_calls_on_client);
>
> There's probably something I'm missing here, but why won't these numbers exactly add up?  And where does the 0.9 factor come from?

Added some commentary.

test/core/iomgr/resource_quota_test.c, line 99 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Suggest using "q" instead of "p" in all of these tests.

Done.

test/core/iomgr/resource_quota_test.c, line 553 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> I don't understand what "pools merged" means.  It looks like the test is creating and then destroying users, one at a time.  What's being merged here?

Renamed.

Comments from Reviewable

Reviewed 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2193 at r5 (raw file):

                                           GRPC_CHTTP2_ENHANCE_YOUR_CALM));
    if (n > 1) {
      /* Since we cancel one stream per destructive reclaimation, if

s/reclaimation/reclamation/

src/core/lib/iomgr/resource_quota.c, line 481 at r3 (raw file):
Previously, ctiller (Craig Tiller) wrote…
> Mostly consistency with current public API's... I can be convinced otherwise.

Thanks for pointing out which functions were part of the public API.  I had lost sight of that -- this makes complete sense.

src/core/lib/iomgr/resource_quota.c, line 108 at r5 (raw file):

    resource_user->links[list].next->links[list].prev =
        resource_user->links[list].prev->links[list].next = resource_user;
    *root = resource_user;

Isn't the root node the start of the list, not the end?  If so, I think this line needs to be moved to rulist_add_head().

src/core/lib/iomgr/resource_quota.c, line 131 at r5 (raw file):
Quoted 5 lines of code…
>     resource_user->links[list].next->links[list].prev =
>         resource_user->links[list].prev;
>     resource_user->links[list].prev->links[list].next =
>         resource_user->links[list].next;
>     *root = resource_user->links[list].next;

It looks like this is removing the root node.  Isn't that the head of the list, not the tail?

src/core/lib/iomgr/resource_quota.h, line 47 at r3 (raw file):
Previously, ctiller (Craig Tiller) wrote…
> consumer, allocator: don't love them because it suggests a sink, and this is actually a collaborator
> usage: mmmh, seems wrong in a way that I'm not sure I can convey right now

I don't quite understand what you mean by "sink" and "collaborator".  Can you elaborate?
As another alternative, how about "allocation"?
Let's chat about this in our 1:1 today.

src/core/lib/iomgr/resource_quota.h, line 63 at r3 (raw file):
Previously, ctiller (Craig Tiller) wrote…
> Benign will be tried before destructive. If we get to doing destructive steps, we'll only do so until benign (or internal) steps are available.

Ah, okay.  In that case, I suggest the following alternative wording:
Only one reclamation will be outstanding for a given quota at a given time.  On each reclamation attempt, the kinds of reclamation are tried in order of increasing invasiveness, stopping at the first one that succeeds.  Thus, on a given reclamation attempt, if internal and benign reclamation both fail, it will wind up doing a destructive reclamation.  However, the next reclamation attempt may then be able to get what it needs via internal or benign reclamation, due to resources that may have been freed up by the destructive reclamation in the previous attempt.

src/core/lib/iomgr/resource_quota.h, line 117 at r3 (raw file):
Previously, ctiller (Craig Tiller) wrote…
> We call malloc(), and ASAN ensures that there's a matching free(). If there's not, it fails the test.

Sorry, I guess my question was unclear: Why do we need this?  Wouldn't asan still detect any problems without this, due to the allocation of the resource_user object itself?  Or, if we really need an allocated data member for some reason, doesn't the `name` field meet this need?

src/core/lib/iomgr/resource_quota.h, line 52 at r5 (raw file):

    free up whatever they can so that the system as a whole can make progress.

    There are three kinds of reclamation that take place:

Suggest adding "(in order of increasing invasiveness)".

test/core/end2end/tests/resource_quota_server.c, line 342 at r5 (raw file):

      NUM_CALLS, cancelled_calls_on_server, cancelled_calls_on_client);

  /* The server call may be cancelled after it's received it's status, but

If I'm understanding this correctly, I suggest the following alternative wording:
The call may be cancelled after the server has sent its status but before the client has received it.

test/core/iomgr/resource_quota_test.c, line 556 at r5 (raw file):

gpr_log(GPR_INFO, "** test_pools_merged_on_resource_user_deletion **");
grpc_resource_quota *q =
grpc_resource_quota_create("test_pools_merged_on_resource_user_deletion");

Please change the name on these two lines as well.

Comments from Reviewable

Review status: 72 of 116 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

src/core/ext/transport/chttp2/transport/chttp2_transport.c, line 2193 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> s/reclaimation/reclamation/

Done.

src/core/lib/iomgr/resource_quota.c, line 481 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Thanks for pointing out which functions were part of the public API.  I had lost sight of that -- this makes complete sense.

Ack.

src/core/lib/iomgr/resource_quota.c, line 108 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Isn't the root node the start of the list, not the end?  If so, I think this line needs to be moved to `rulist_add_head()`.

Done.

src/core/lib/iomgr/resource_quota.c, line 131 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >     resource_user->links[list].next->links[list].prev =
> >         resource_user->links[list].prev;
> >     resource_user->links[list].prev->links[list].next =
> >         resource_user->links[list].next;
> >     *root = resource_user->links[list].next;
>
> It looks like this is removing the root node.  Isn't that the head of the list, not the tail?

I think I got myself confused after the last round of review feedback.

src/core/lib/iomgr/resource_quota.h, line 63 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Ah, okay.  In that case, I suggest the following alternative wording:
>
> Only one reclamation will be outstanding for a given quota at a given time.  On each reclamation attempt, the kinds of reclamation are tried in order of increasing invasiveness, stopping at the first one that succeeds.  Thus, on a given reclamation attempt, if internal and benign reclamation both fail, it will wind up doing a destructive reclamation.  However, the next reclamation attempt may then be able to get what it needs via internal or benign reclamation, due to resources that may have been freed up by the destructive reclamation in the previous attempt.

Done.

src/core/lib/iomgr/resource_quota.h, line 117 at r3 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Sorry, I guess my question was unclear: Why do we need this?  Wouldn't asan still detect any problems without this, due to the allocation of the resource_user object itself?  Or, if we really need an allocated data member for some reason, doesn't the `name` field meet this need?

resource_user gets inlined in other data structures... name probably fills that need now however (it did not at the time of authoring).

src/core/lib/iomgr/resource_quota.h, line 52 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> Suggest adding "(in order of increasing invasiveness)".

Done.

test/core/end2end/tests/resource_quota_server.c, line 342 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> If I'm understanding this correctly, I suggest the following alternative wording:
>
> The call may be cancelled after the server has sent its status but before the client has received it.

Done.

test/core/iomgr/resource_quota_test.c, line 556 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
> >   gpr_log(GPR_INFO, "** test_pools_merged_on_resource_user_deletion **");
> >   grpc_resource_quota *q =
> >       grpc_resource_quota_create("test_pools_merged_on_resource_user_deletion");
>
> Please change the name on these two lines as well.

Done.

Comments from Reviewable

This looks great!  Assuming the tests pass, feel free to merge.

Reviewed 43 of 44 files at r6, 5 of 5 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

test/core/end2end/tests/resource_quota_server.c, line 342 at r5 (raw file):
Previously, ctiller (Craig Tiller) wrote…
> Done.

Please re-add the "this means that we should see strictly more failures on the client than on the server" part.

Comments from Reviewable

Looks like something broke during the review... will dig in and get the tests passing and then ping this thread.

6476170 86cb138 and 0f7eb30 are worth cursory review.
There's still one gremlin I'm tracking down.

I think that's it (though I'll peek again in the morning): 7a40897 and 693d394 address the last bugs I've seen today.

In the future, please check for Node test failures when changing core code, and especially iomgr code.


Added PAYE and VAT methods in sml approach

Added PAYE and VAT methods

Cucumber features added

Please resolve the merge conflicts before re-submitting the PR.
As a general rule, it is also helpful if you squash commits as part of preparing the PR.


Apache client throws exception for BASIC auth in POST

In the HttpPostSpec test, the '[#client] POST (BASIC) /basic: returns content' test fails for the Apache client but passes for the Java client. The exception thrown is:
java.lang.RuntimeException: org.apache.http.client.ClientProtocolException
    at groovyx.net.http.optional.ApacheHttpBuilder.exec(ApacheHttpBuilder.java:292)
    at groovyx.net.http.optional.ApacheHttpBuilder.doPost(ApacheHttpBuilder.java:347)
    at groovyx.net.http.HttpObjectConfigImpl.nullInterceptor(HttpObjectConfigImpl.java:39)
    at groovyx.net.http.HttpBuilder.post(HttpBuilder.java:693)
    at groovyx.net.http.HttpPostSpec.[#client] POST (BASIC) /basic: returns content(HttpPostSpec.groovy:165)
Caused by: org.apache.http.client.ClientProtocolException
    at org.apache.http.impl.client.InternalHttpClient.doExecute(InternalHttpClient.java:186)
    at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:71)
    at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:220)
    at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:164)
    at groovyx.net.http.optional.ApacheHttpBuilder.exec(ApacheHttpBuilder.java:289)
    ... 4 more
Caused by: org.apache.http.client.NonRepeatableRequestException: Cannot retry request with a non-repeatable request entity.

which seems related to this SO post (http://stackoverflow.com/questions/8222929/httppost-failed-due-to-cannot-retry-request-with-a-non-repeatable-request-entit) about repeatable entities. Our HttpEntity implementation is "not" repeatable, but I am not sure whether that is intentional or just "because".

They can be repeatable, the ApacheHttpBuilder class would have to buffer the InputStreams and only throw them away once the response starts. I punted on this when implementing it hoping it wouldn't be a problem.
The correct solution is to implement that buffered input stream, storing the contents in an array of ByteBuffer up to a maximum limit configured HttpObjectConfig.Client or HttpObjectConfig.Execution. Default out of the box would be something reasonable like 4 MB. Once 4 MB had been buffered the buffer would signal to the Apache HttpClient that the request is no longer repeatable and stop buffering. In 99.9% of the cases, the client won't be uploading large amounts of data and this is acceptable.
I'm guessing that the Java HttpUrlConnection is doing this internally by default.

This seems like a minor issue. I should be able to take this one up once I am done with testing/documenting the verbs if you don't pick it up first.

I have opened #56 - just noting as a related issue.

Issue seems to be test related and is resolved in my ersatz testing branch - closing this for now.

Doh! Nope, this is still an issue... it seems to fail during full suite test runs.

This has been fixed in development and will be merged with the 0.14.0 release.

This has been addressed in the 0.14.0 release (development branch)


Rendering tweaks

Thought we could have an issue to collect up tweaks to rendering that could be done.

[ ] Make a more generic way of deciding the height of operation. For swaps we currently use the square root of the distance between the inputs.
[ ] Positioning nodes vertically: When a node with low height appears in a slice which is tall it looks odd. We could make sure that the node renders at its normal height and/or render it closer to the top or bottom of the slice (depending if number of inputs > number of outputs) instead of always centering it
[ ] Tweaking bezier curves. With the bigger diagrams, some of the bezier curves don't look great. Not really sure how to fix it but someone could probably mess with it for a bit and make it slightly better

We can add some more things here if we think of them

This is all solved by vertical layout


update submodule model

new hash for submodule/model, works now with updates docs.

test with https://github.com/code-kern-ai/refinery-submodule-model/pull/1


NoSuchMethodException

Commit https://github.com/scylladb/scylla-cdc-source-connector/commit/80b7117af97ce6bda360319d70968a97252d826d introduced a bug by updating flogger dependency version from 0.5.1 to 0.7.4.
Indeed, these lines of code https://github.com/scylladb/scylla-cdc-source-connector/blob/master/src/main/java/com/scylladb/cdc/debezium/connector/ScyllaConnector.java#L36-L38 trigger
java.lang.NoSuchMethodException: com.google.common.flogger.backend.log4j.Log4jBackendFactory.getInstance()

@wyfo For us to quicker solve the problem, could you specify the environment you launch the connector in? (whether using the "fat jar" (jar-with-dependencies) or "Confluent Hub ZIP package", version of Kafka (Confluent/open-source), maybe some non-standard logging configuration)

I hit the issue while writing #28, in a basic test I had written for the occasion to test the SSL connection, so JUnit, with maven install plugin to install locally the modified https://github.com/scylladb/scylla-cdc-java dependency.
Anyway, com.google.common.flogger.backend.log4j.Log4jBackendFactory#getInstance doesn't exists in flogger 0.7.4 https://javadoc.io/doc/com.google.flogger/flogger-log4j-backend/0.7.4/com/google/common/flogger/backend/log4j/Log4jBackendFactory.html (vs https://javadoc.io/doc/com.google.flogger/flogger-log4j-backend/0.5.1/index.html where it exists).
I know that https://github.com/scylladb/scylla-cdc-java dependency still uses version 0.5.1, so maybe some maven black magic should normally happen (I'm quite a noob concerning maven), but it did not in my test and I had this error.

Thanks for raising this issue. I must have not noticed it when running the connector. I have been playing around with this recently and here's what I have found so far:
Silver lining is that somehow most of the logging still works. I still can see flogger logs originating from scylla-cdc-base, although in a different format from other logs. Possibly because it goes through different logging backend since I messed up setting the property.
I tried both fixing the property and reverting to state before faulty commit, but it turns out there's a little larger issue with that.
Right now it results in swapping NoSuchMethodException with ClassCastException (this is Flogger 0.5.1 with fixed property):
class shaded.com.scylladb.cdc.driver3.common.flogger.util.StaticMethodCaller:
  cannot cast result of calling 'com.google.common.flogger.backend.log4j.Log4jBackendFactory#getInstance' to
  'shaded.com.scylladb.cdc.driver3.common.flogger.backend.system.BackendFactory':
    java.lang.ClassCastException: Cannot cast com.google.common.flogger.backend.log4j.Log4jBackendFactory to
    shaded.com.scylladb.cdc.driver3.common.flogger.backend.system.BackendFactory

It seems that because scylla-cdc-driver3 shades and relocates its flogger dependencies it (correctly) does not recognize that backend provided with cdc connector dependencies could be cast to BackendFactory in different circumstances.
I'll see what happens locally when I unshade this dependency and I'll try to use this opportunity to switch to flogger-log4j2-backend version 0.5.1 at the same time.

Fixed as part of https://github.com/scylladb/scylla-cdc-java/pull/73.


Use peep instead of pip (bug 1118379)

Fixes bug 1118379

@andymckay r?

I think this should also copy the peep.py file to the repo and verify it, otherwise peep itself is the weakest link in the chain. See the peep README for this suggestion.

I was going to suggest install peep off of pyrepo and then you don't need to install the rest of pyrepo. Embedding it works too.

This is currently broken, I believe because of the incompatibility of peep and pip7 (eg https://github.com/erikrose/peep/pull/94)
Also, I'm not sure we really need/want to use peep. It would allow us to not need pyrepo anymore, installing straight from pypi. However, we'd then have other issues (what if a package is badly updated on pypi as it happened for the python-memcached 1.53 wheel which is in fact now a 1.55 package? Or if someone removes an old version or deletes a package from pypi?).
So I believe we need to keep pyrepo, and we thus don't need peep. Thoughts @andymckay @robhudson?

I think I'd still rather have peep, even if we stick with pyrepo. We thought we were getting all of our packages from our wheelhouse before, and things still broke, and were very difficult to diagnose. If we can manage to get this working without making our lives too difficult in the future, I think it's worth it.

I agree with @kmaglione, peep is worth it.

Closing for now: the peep bug is still open.
If this is reopened, it'll also need some more documentation around how to maintain the requirements files (updating, adding a new requirement, adding the appropriate hashes). Might need a lot of rework if https://github.com/mozilla/olympia/pull/795 makes it to master (it got reverted in https://github.com/mozilla/olympia/commit/fa65808a2d5e009f6ffdd7de10df989307928360)


Darken grays

Moves grays to be AA accessible at small sizes.
#555 -> #333
#bababa -> #aeaeae
/cc @collino

Deploy preview for gestalt ready!
Built with commit 59264c65ef39c88f6baa53b18b9ea8f0dc06db3b
https://deploy-preview-276--gestalt.netlify.com

Warnings

:warning:

:memo: CHANGELOG - Please add a CHANGELOG.md entry for your changes

:warning:

:file_folder: Bundlesize - Bundle size increase detected, please review

Filename
Size Diff
Gzip Diff
Prev Size
Current Size
Prev Gzip
Current Gzip

gestalt.js
:small_red_triangle: 5.1%
:small_red_triangle: 4.9%
255 KB
268.09 KB
56.8 KB
59.57 KB

gestalt.es.js
:small_red_triangle: 5.2%
:small_red_triangle: 4.9%
252.37 KB
265.46 KB
56.42 KB
59.19 KB

  Generated by :no_entry_sign: dangerJS

Codecov Report

Merging #276 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #276   +/-   ##
=======================================
  Coverage   70.24%   70.24%
=======================================
  Files          46       46
  Lines         820      820
  Branches      189      189
=======================================
  Hits          576      576
  Misses        172      172
  Partials       72       72

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 990f132...eabd370. Read the comment docs.


[KYLIN-5215] Fix  Kylin pushdown query and Cube query is inconsistent

Fix columnMetas information returned by Kylin pushdown query and Cube query is inconsistent
Proposed changes

Delete toUpperCase  from  PushDownRunnerSparkImpl.executeQuery

            columnMetas.add(new SelectedColumnMeta(false, false, false, false, nullable, true, Integer.MAX_VALUE,                     fieldList.get(i).getName().toUpperCase(Locale.ROOT), fieldList.get(i).getName().toUpperCase(Locale.ROOT), null, null,                     null, fieldList.get(i).getPrecision(), fieldList.get(i).getScale(), fieldList.get(i).getDataType(),                     fieldList.get(i).getDataTypeName(), false, false, false));
Checklist

I have create an issue on Kylin's jira, and have described the bug/feature there in detail

Thx for the pr, will come back with more comments in a few days.

Thx for the pr, will come back with more comments in a few days.

pending on design discussion in https://issues.apache.org/jira/projects/KYLIN/issues/KYLIN-5215


Adding a new feature for reward sketching

Is your feature request related to a problem? Please describe.
Being able to annotate rewards is missing in this label studio. This reward annotation is useful for training RL agents.
Describe the solution you'd like
I'd like to have a way to create reward sketches like done here https://sites.google.com/view/data-driven-robotics/ on video data.
Describe alternatives you've considered
I have not seen anything close to this in the repo.
Additional context
These reward sketches are useful to have to train RL agents. This is the closest you can get to have a supervised signal for robotics.

@ankurhanda What kind of reward sketches do you want to have?
LS has ratings, checkboxes and text areas, so you can have something like this for video.

@makseq I'd like to draw a continuous function with X-axis being the frame number and Y-axis being the reward. To clarify what that means check the reward sketching section in https://sites.google.com/view/data-driven-robotics/ where you slide your mouse along and it draws the curves for you.

Thanks! Now I've got it. Very cool feature!

Do you think it is worth adding that in label-studio?
I implemented it in python already (using matplotlib) but it's fairly simple and often requires more clicking.

some of this is inspired from https://github.com/yuma-m/matplotlib-draggable-plot/blob/master/draggable_plot.py

Thank you very much! But we need to re-implement it in JS React for LS.

Yes, sure. Let me know if you ever get to it. I'm happy to do testing for you. Thanks a lot for listening.

https://labelstud.io/tags/number.html#main - we introduced Number tag, it can help here.


shake to undo gets triggered

I'm filling out an input field, before starting a game that is played by shaking the phone. Unfortunately, on iOS, this triggers the « Shake to Undo».
How can I add a event.stopPropagation(); to the original event, to stop this behaviour?
Thanks

AFAIK Shake-to-undo is OS level. I dont think you can control it from the browser.
Why don't you have two HTML pages? :)
Get the input in the first. Enter/submit gets you to the second page where you have the game.
Question is not directly related to the library so I am closing it as an issue. Feel free to continue the conversation though.
Cheers,
D.

hmm, unfortunately I need to stay on the same page.
I found some sources where they said that I could prohibit this with stopPropagation somehow.
I'll do some tests.

A quick search:
https://stackoverflow.com/questions/6520125/how-to-disable-ios-shake-to-undo-in-a-webapp The second reply here can be use full.
Cheers,
D.

Yes, I finally used the third answer, which worked perfectly (I dont need emoji)
https://stackoverflow.com/a/43481473/1779355

Glad that it worked!


Provide Impersonation feature on WebUI

Description
Towards improving Permissions management, permissions debugging and providing the same features from kubectl gs to Happa for customers on SSO.
TODO

[ ] Create Specs
[ ] Implement
[ ] Test
[ ] Add Documentation
[ ] inform Customers

closing


Persist Remote Extensions

Just curious, have you been able to figure out a good way of persisting remote vscode extensions when re-recreating the container? I've been able to add them from the interface, but can't find a way to install them from the CLI. This command doesn't appear to work:
> code --install-extension ms-dotnettools.csharp

No installation of Visual Studio Code Stable was found.
Install it from https://code.visualstudio.com and try again.

If you already installed Visual Studio Code and we didn't detect it, run `code version use stable --install-dir /path/to/installation`

I tried using that --cli-data-dir tag via a docker ENV variable VSCODE_CLI_DATA_DIR, but it doesn't appear to persist any extension related info.
Note, I did modify your existing dockerfile and got it to work for a windows docker image, so I did end up finding the extensions stored in %userprofle%\.vscode-server (not sure what the equivalent would be in the linux world), so I suppose we could just link up a volume to that directory, but that seems a little hacky. I'd prefer to setup some default extensions during the build process so I have a consistent environment whenever it's torn down and recreated.
Appreciate you putting this example repo together, really made it easy to get up and going.

FYI, I did have luck with getting this to work by using another volume for that .vscode-server folder, but hoping you might have found a better way to install these during the build process.

Have some additional findings that I wanted to share:
Using the docker diff tool I was able to find the extensions directory to mount .vscode-server for Linux containers: /root/.vscode-server. Where as windows would be: C:/Users/ContainerAdministrator/.vscode-server. So creating another docker volume mount for just this folder seems to work to persist extensions between runs. Nice for persisting what a user installs during and in-between container runs, but still lacking if we want a list of extensions to install by default w/o user interaction (i.e. during that very first build & run).
FYI, this is how I amended your docker run command to add this new volume mounting point:
@ECHO OFF

SET IMAGE_NAME=alexanderdna/container-tunnel
SET INSTANCE_NAME=alexanderdna-container-tunnel

docker start %INSTANCE_NAME% 2>NUL || docker run ^
    --detach ^
    --volume %INSTANCE_NAME%:/home/workspace ^
    --volume %INSTANCE_NAME%-server:/root/.vscode-server ^
    --workdir /home/workspace ^
    --name %INSTANCE_NAME% ^
    %IMAGE_NAME%:latest

It also seems beneficial to use the --cli-data-dir CLI switch which seems to remember things like the tunnel workspace name in-between container runs (stored under /root/.vscode-cli by default), otherwise, when re-creating the container, it tends to rename with an incremented number at the end since it thinks it's a completely separate tunnel. I ended up modifying your Dockerfile with the environment variable VSCODE_CLI_DATA_DIR:
FROM ubuntu:22.04

ENV VSCODE_CLI_DATA_DIR=/home/workspace/vscode-cli-data

RUN apt-get update && \
    apt-get install -y git curl && \
    apt-get clean && \
    rm -rf /var/lib/apt/lists/* && \
    curl -sL "https://code.visualstudio.com/sha/download?build=stable&os=cli-alpine-x64" \
        --output /tmp/vscode-cli.tar.gz && \
    tar -xf /tmp/vscode-cli.tar.gz -C /usr/bin && \
    rm /tmp/vscode-cli.tar.gz

CMD [ "code", "tunnel", "--accept-server-license-terms" ]

The ENV route seems ideal, vs having to spin up and manage a completely separate volume. So I wonder if we can add some sort of configuration to also redirect that .vscode-server folder to our existing volume mount so we can reduce our mounts down to just the one. Found issue #472, but I'm not sure yet how we can apply that to this sort of setup, as that JSON config that is mentioned seems like something I would need to setup from my local VSCode install.

Hi! Thank you for your support and suggestions. I'm going to look for some solutions once I get a little of free time this week.

Thanks for this issue, it helped me persist things on my end. I don't do volumes I just bind mounts instead. I also changed the dockerfile to a different image so I can use a python interpreter, and then also added naming for the tunnel which seems to help with persitence.
Dockerfile that is in the vs-code-tunnel/ folder (that sits on the same level as the below docker-compose):
FROM python:3.10-bullseye

ENV VSCODE_CLI_DATA_DIR=/home/workspace/vscode-cli-data

RUN apt-get update && \
    apt-get install -y git curl && \
    apt-get clean && \
    rm -rf /var/lib/apt/lists/* && \
    curl -sL "https://code.visualstudio.com/sha/download?build=stable&os=cli-alpine-x64" \
        --output /tmp/vscode-cli.tar.gz && \
    tar -xf /tmp/vscode-cli.tar.gz -C /usr/bin && \
    rm /tmp/vscode-cli.tar.gz

CMD [ "code", "tunnel", "--accept-server-license-terms", "--name", "name-of-tunnel"]

And then my docker-compose.yml:
version: "3"
sevices:
  vs-code-tunnel:
    container_name: vs-code-tunnel
    image: vs-code-tunnel:latest
    build:
      dockerfile: ./vs-code-tunnel/Dockerfile
      context: ./
    volumes:
      - /mypath/vs-code-tunnel/workspace:/home/workspace
      - /mypath/vs-code-tunnel/data:/root
      - /mypath/projects:/projects
    restart: unless-stopped

This seems to persist pretty well (extensions, windows, interpreter) even with docker restarts and everything


feat: add crawl script & CI step; update broken links

Adds a new bin/crawl.ts script that will crawl the /public directory

Note: If you're running this manually, you must run the build script first!

This script will add a warning if an internal link contains the https://developers.cloudflare.com/foobar/... prefix. These should be rewritten into absolute URL pathnames instead; eg /foobar/....
The script will add an error if the anchor link or image source is not found in the /public directory.

Note: Even if the pathname is redirected in the live site, an error is still shown! Our website should always be pointing to the latest URLs for each document, leaving redirects for external properties' links to our pages.

Here's an example view of the script's output:

Each block starts with the output file, which correlates to the URL/source file.
The indented lines under each filename represent the offending link/image target.
The symbols delineate errors vs warnings.
If any errors are present, the process exits with a non-zero code, for CI.

In this PR, I updated as many broken links and images as I could manually.
There are 8 broken links/images remaining, which will require others' input to fix.
I could not locate any redirects or original file for the paths that remain. For example, /cloudflare-one/static/secure-origin-connections/share-new-site/applications.png never existed & was broken in the old/previous engine too.

This PR will continue to fail CI until all 8 remaining errors are corrected. See here for example CI output.

@soheiokamoto, from your approval, does that mean that you'll be able to update the logs source to match these new links?
(so we don't have to re-do some of this work every time there are changes to the logs docs?)

@soheiokamoto, from your approval, does that mean that you'll be able to update the logs source to match these new links? (so we don't have to re-do some of this work every time there are changes to the logs docs?)

Yes, I've made a PR to match the change on Log fields pages from this PR.


New Data Source from ADS - Status

New status data source.

Might be a good home for Sensor health and status

I like the concept, tbd based on mappings (may merge w/ https://github.com/mitre-attack/attack-datasources/pull/24, i see the difference (Sensor health and status might map to this better for Impact/DE, whereas Config might be more DE)


Apotheosis & Passive Skill Tree

crash-2023-08-03_15.29.06-client.txt
I keep getting this crash. I've talk with the owner of PST and he wanted me to reach out to you.

Passive skill tree is invoking setSockets before the socket affix is loaded via datapack registry (on world start), which crashes.


Rename project organizer "folders" to "collections"

Steps

Go to the Project Organizer

Expected
Button in the status bar will say, "New Collection"
Actual
Button in the status bar says, "New Folder"
Now that the OSF can make folders in the grid view, the things in the project organizer should be referred to as Collections (and Smart Collections) to keep people from  getting confused. This will need changing in all the relevant status bar buttons and in the legend.

The upcoming PR from @caneruguz takes care of this, yes?

Yes. Is that slated for 36?

If it is related to this #2606 then yes!

:+1:

it is

Fixed by https://github.com/CenterForOpenScience/osf.io/pull/2606

@GaryKriebel this issue can be closed.


Add Form UI demos to MixItUp Core

Several users asking for examples of using <select> options, checkboxes etc with MixItUp core via API. Could do with some additional demos.

Selects, checkboxes and radio buttons demos now added to MixItUp v3.2.2


Test three-day notification

Use this issue to test the three-day notification currently set in .github/workflows/due-date-notifications-via-comments.yml.
Due date: 2024-06-18

This issue is due in 3 day(s).

This issue is due in 3 day(s).

This issue is due in 2 day(s) on 2024-06-18.

This issue is due in 2 day(s) on 2024-06-18.

This issue is due in 2 day(s) on 2024-06-18.

This issue is due in 2 day(s) on 2024-06-18.

Test passed.


v1.14 Backports - background node sync improvements

[x] #32577 (@marseel)
[x] #32630 (@marseel)

Once this PR is merged, a GitHub action will update the labels of these PRs:
 32577 32630

/test-backport-1.14

/test-backport-1.14

I understand deployments are proposed because the PR was initially opened against main, by mistake. Deployments are not necessary for this PR, I'll just go ahead, ignore them and merge.


Fix all false column bug

@erp12 this should address #326. Please let me know if this is good enough!
I don't have g/schema though, so I left that part of the test. If that's your own function, it sounds useful! Would you like to add that in?


Initial addition of civic summaries to ggd

Before creating a pull-request please run ggd check-recipe path/to/recipe-dir/.
ggd can be install via: pip install -U git+git://github.com/gogetdata/ggd-cli.git
If your pull-request errors on travis-ci, follow the link and read the log carefully;
it should give information about what went wrong.

@fakedrtom,
Your forked ggd repo is quite a bit out of date compared to the current ggd repo. Please update your forked repo:
Navigate to your local forked repo.


Updated to redirect any HTTP request to HTTPS. Fixes #102

This should fix #102 where I suggested to use HTTPS only.

Awesome! Thank you for your contribution!


Fixed Date Selector to reflect local time

Issue #: /bcgov/entity#3863
Description of changes:

Refactored the Min and Max Date for allowed for the Date selector to reflect Local Time not UTC time in the selector.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the bcrs-entities-create-ui license (Apache 2.0).

3 approvals, will merge!


Improve usability: welcome screen with first steps

has been developed on features/StartWindow

has been developed on features/StartWindow


All feedback pop-ups refer to "UK Contracts Finder"

I've checked multiple sources and each time I select the "Contact OCP helpdesk" button on the bottom right, the pop-up message always refers to the dataset as "UK Contracts Finder".
The heading should refer to the dataset title the user is on when they click the "Contact OCP helpdesk" button

fixed

This is now missing the country e.g. for
"Australia Department of finance" only "Department of finance" is coming up.
"Afghanistan Records" only "Records" is coming up
I think we need to include the name of the country

Done and deployed.

I've carried out spot checks. Looks good :)


Fix a 404 on a link to the WPE site

Looks like it's not living at Igalia anymore - but there's a new nice page under webkit.org

@jornh thanks for the contribution! Could you update the message on your commit (not the PR) to include the footer: Change-type: patch ?

@chrisys yes, sure! By “into to footer“ I’m guessing you mean it has to go into any subsequent commit line like I did in my second commit https://github.com/balena-io-projects/balena-wpe/pull/63/commits/898b99ce9e1829d94d6219512aa9b6175da0e1df ?

@balena-ci retest

@balena-ci retest


build(backend): Update Rust version

A new Rust version available.

I bumped cargo update -p time to resolve the issue. Few others (see rust-lang thread seems to have faced the same issue).
error[E0282]: type annotations needed for `Box<_>`
  --> /Users/daviddalbusco/.cargo/registry/src/index.crates.io-6f17d22bba15001f/time-0.3.34/src/format_description/parse/mod.rs:83:9
   |
83 |     let items = format_items
   |         ^^^^^
...
86 |     Ok(items.into())
   |              ---- type must be known at this point
   |
help: consider giving `items` an explicit type, where the placeholders `_` are specified
   |
83 |     let items: Box<_> = format_items


use stdlib::ensure_packages() and require stdlib >= 9

Resolves this warning:
Warning: This function is deprecated, please use stdlib::ensure_packages instead. at ["/etc/puppetlabs/code/environments/production/modules/ccs_software/manifests/pre.pp", 16]:["/etc/puppetlabs/code/environments/production/modules/ccs_software/manifests/init.pp", 119]

@glennmorris This should technically require a major version bump.

@glennmorris We are already using stdlib 9, so its no change for the control repo.  Per semvar, the next version bump for this module should be a major version.


Avoid tight coupling to Cargo in frb2.0

In the current version, the flutter_rust_bridge_codegen invokes Cargo directly to accomplish tasks. It reads various files from various directories to pick up inputs with minimal configuration. This is great for being easy and idiomatic for Rust-centric projects, but for polyglot/monorepo projects built with tools like Bazel, it is less convenient. It Would Be Nice™ if frb 2.0 could be friendly to alternative build systems. In Bazel's case (and other build systems with the similar design), that mostly means designing the tools to support an explicit connection between output files and input files.

That looks interesting! I totally agree FRB should not be tighted to build systems.
Since I am not super familiar with bazel, I guess maybe I will firstly release a 2.0-prerelease (such that the APIs are not stablized yet), and feel free to provide a Bazel example, and we can discuss the details about how to do it :)

The main relevant idea of this family of build systems is having explicit input and output files. Take every file input on as a parameter, only output to files specified in parameters.
So, practically speaking:
--rust-crate-dir=<dir> would become something like --rust-crate-spec=<path/to/Cargo.toml>
--dart-root= would become --pubspec-yaml=<path/to/pubspec.yaml>,
--dart-output=... would become a family of options like --dart-web-output, --dart-io-output, ... one for each file.
--wasm would become something like --wasm-out=<path/to/mod_bg.wasm> and --wasm-js-out=<path/to/mod.js>,
The secondary idea in bazel is sandboxing by default. Any internet downloads are predeclared outside of the build step, and build steps don't require internet access and avoid using external helper tools via PATH. This is easiest to achieve if the frb binary contains its functionality as much as possible directly rather than orchestrating/scripting other tools.
I'll definitely take a look at the 2.0 alpha when there is one. Just thought I'd drop this issue as a note since you mentioned thinking about a 2.0 elsewhere, and this kind of refactoring is easiest to handle in a big rewrite.

I see, that looks quite reasonable!

Btw,

--rust-crate-spec=<path/to/Cargo.toml>

I guess it is still rust-crate-dir, because FRB scans the whole crate's source files. For example, if you change something in your api.rs (or other files, in v2), it should generate something different.

The code & doc for 2.0 is on master branch now, feel free to have a look! (The 2.0.0-dev.1 is not released yet, but that is unrelated to this issue)

(I renamed the title, since it seems that FRB is not only coupled to cargo, but also coupled to dart run etc)

As far as I can tell, this would be a significant project. FRB has a lot of features make things convenient and the underlying components would need to be extracted into a modular "plumbing" layer to make it feasible to be driven by bazel/ninja/whatever.

I see. Btw, the v2 code is more modularized, so I guess most things needed here are in the config module, polish module, etc.


Unable to send events after update libcurl to v8.1.0

After updating libcurl to 8.1.0 the Laravel SDK is not able to send events to Sentry, it fails with the following error:
[sentry-laravel] error: Failed to send the event to Sentry. Reason: "HTTP/2 stream 1 was reset for "https://o388665.ingest.sentry.io/api/5461612/envelope/".". | {"exception":{},"event":{}}

The PHP SDK also fails to send events, but silently.
https://github.com/curl/curl/releases/tag/curl-8_1_0
https://github.com/curl/curl/releases/tag/curl-8_0_1
Current workaround: downgrade to v8.0.1.

The problem is in \Http\Client\Common\Plugin\DecoderPlugin::handleRequest as is adding header 'TE'
$request = $request->withHeader('TE', $encodings);
with values
"TE" => array:3 [               0 => "gzip"               1 => "deflate"               2 => "chunked"             ]
And RFC for HTTP/2 specifies The only exception to this is the TE header field, which MAY be present in an HTTP/2 request; when it is, it MUST NOT contain any value other than "trailers"."
https://httpwg.org/specs/rfc9113.html#rfc.section.8.2.2

Can confirm - either remove TE headers, or switch protocol to 1.1 in
vendor/symfony/http-client/CurlHttpClient.php
public function request

I opened an issue over at https://github.com/php-http/client-common/

When code reaches DecoderPlugin, the protocol is set to 1.1, it is only switched to HTTP2 at CurlHttpClient, after the TE headers are already set
} elseif (\defined('CURL_VERSION_HTTP2') && (\CURL_VERSION_HTTP2 & CurlClientState::$curlVersion['features']) && ('https:' === $scheme || 2.0 === (float) $options['http_version'])) {             $curlopts[\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_2_0;         }
In my opinion, the CurlHttpClient should verify it can switch to HTTP2 with the headers, or manage them accordingly after switching to HTTP2, but it is up to debate.

Appreciate the feedback, but let’s move this over to the repository in question.

We released v3.19.1 that enforces HTTP/1.1 if compression is enabled.

if someones stumbles on the issue and upgrade to v3.19.1 is not helping then try to update symfony/http-client to v 5.4.16 at minimum.
HttplugClient in earlier version forced using http_version with null value, which results in setting \CURL_HTTP_VERSION_2_0 as default.
problem
https://github.com/symfony/http-client/blob/v5.4.15/HttplugClient.php#L262
fix
https://github.com/symfony/http-client/blob/v5.4.16/HttplugClient.php#L266


Can't login to my desktop session

I've started the container and pointed my browser to http://myhost:6080/vnc.html
All I see is the default noVNC login window asking(?) me to enter host/port/password. I believe it supposed to automatically open up the lxde session. I am doing following to start the container
$ docker run -i -t -p <IP_ADDRESS>:6080:6080 dorowu/ubuntu-desktop-lxde-vnc

Following is the stdout log from the container
/usr/lib/python2.7/dist-packages/supervisor/options.py:295: UserWarning: Supervisord is running as root and it is searching for its configuration file in default locations (including its current working directory); you probably want to specify a "-c" argument specifying an absolute path to a configuration file for improved security.
  'Supervisord is running as root and it is searching '
2016-07-26 18:50:48,224 CRIT Supervisor running as root (no user in config file)
2016-07-26 18:50:48,225 WARN Included extra file "/etc/supervisor/conf.d/supervisord.conf" during parsing
2016-07-26 18:50:48,259 INFO RPC interface 'supervisor' initialized
2016-07-26 18:50:48,259 CRIT Server 'unix_http_server' running without any HTTP authentication checking
2016-07-26 18:50:48,259 INFO supervisord started with pid 28
2016-07-26 18:50:49,263 INFO spawned: 'xvfb' with pid 35
2016-07-26 18:50:49,266 INFO spawned: 'lxsession' with pid 36
2016-07-26 18:50:49,270 INFO spawned: 'x11vnc' with pid 37
2016-07-26 18:50:49,279 INFO spawned: 'novnc' with pid 38
2016-07-26 18:50:50,307 INFO success: xvfb entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-07-26 18:50:50,307 INFO success: lxsession entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-07-26 18:50:50,307 INFO success: x11vnc entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-07-26 18:50:50,307 INFO success: novnc entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)

Hi,
Please connect to http://myhost:6080/ or http://myhost:6080/vnc_auto.html

That worked!


Add upsert search attributes

Pull Request Test Coverage Report for Build 250

53 of 79 (67.09%)  changed or added relevant lines in 7 files are covered.
534 unchanged lines in 15 files lost coverage.
Overall coverage decreased (-0.2%) to 73.749%

Changes Missing Coverage
Covered Lines
Changed/Added Lines
%

internal/workflow.go
6
8
75.0%

workflow/workflow.go
0
3
0.0%

internal/internal_task_handlers.go
1
11
9.09%

internal/internal_event_handlers.go
7
18
38.89%

Files with Coverage Reduction
New Missed Lines
%

internal/propagation.go
2
92.59%

internal/workflow_testsuite.go
2
83.46%

internal/client.go
3
91.78%

internal/common/backoff/retrypolicy.go
3
91.57%

internal/internal_public.go
3
0.0%

internal/error.go
5
84.68%

internal/internal_workflow_client.go
7
68.62%

workflow/session.go
12
0.0%

internal/internal_event_handlers.go
19
52.62%

internal/internal_decision_state_machine.go
30
79.16%

Totals

Change from base Build 248:
-0.2%

Covered Lines:
8299

Relevant Lines:
11253

💛  - Coveralls


Improve usability of AWS S3 instructions

As an experienced programmer and linux user, but brand new user to AWS systems, I was totally lost.

When I saw code blocks, I assumed I could be following along, but that lead to issues since I didn't yet have a storage bucket to operate on.
Additionally, AWS is a massive system and I had no idea where to find the starting pages for the S3 instructions. Direct links were added to help future students. Further adjustments could include instruction on searching for S3 in the toolbar.
I promise I'm not stupid, but I am brand new to the AWS system and I (and likely other students too) need additional help to get oriented in this new space.

Great feedback. Thanks James. I will probably go and try and improve and clarify the S3 instruction with some of the suggestions you have made here and then close this pull requests.

We should probably have an instruction item that is an intro do AWS

I have created a new topic AWS Intro, and cleaned up the S3 topic.
I believe this covers everything that you mentioned in the PR. Let me know if I missed something.


serverless/appsec/httpsec: support base64 encoded request body

What does this PR do?

Add support for base64 encoded bodies as input to the runtime api proxy.

Motivation
Extend support of appsec in the serverless datadog extension.
Additional Notes
Had to fix the test api-gateway-appsec.json because it was incorrectly telling that the body was in base64.
Possible Drawbacks / Trade-offs

Describe how to test/QA your changes
They were already tests about this be we never bothered to understand that the body was in base64.
Also added a unit test.
Reviewer's Checklist

[x] If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
[ ] Use the major_change label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.
[ ] A release note has been added or the changelog/no-changelog label has been applied.
[x] Changed code has automated tests for its functionality.
[x] Adequate QA/testing plan information is provided if the qa/skip-qa label is not applied.
[x] At least one team/.. label has been applied, indicating the team(s) that should QA this change.
[ ] If applicable, docs team has been notified or an issue has been opened on the documentation repo.
[ ] If applicable, the need-change/operator and need-change/helm labels have been applied.
[ ] If applicable, the k8s/<min-version> label, indicating the lowest Kubernetes version compatible with this feature.
[ ] If applicable, the config template has been updated.

/merge

/merge


Remove tagging_user from fb query

This was causing an auth error for @rafael-atticlabs. Unclear why, but it's not necessary in first place.

TBR


Consider converting RouterService.apply's do{} to flatMap

According to YourKit, about 8% of our allocations are occurring in
io.buoyant.router.RoutingFactory$RoutingService$$anonfun$apply$3$$anonfun$apply$4.apply(Object)
within a flatMap. I think this is RouterFactory.scala:98, which is a transform of a Req to a Future[Rsp]. We should talk about rewriting this to something more procedural/less allocate-y. (if this does kill 8% of our allocations, it'd probably be worth a blog post, too)

Are you able to see what is being allocated in that method?  Looking at the code I see allocations of two closures (one passed to rescue and one passed to ensure) which probably show up a PartialFunction and Function0.
Those closures can't just be val'd up though, because they capture some local variables.
If scala isn't smart about how for is desugared, there might be allocations of Function1 as well from the desugared flatMap calls.  Those should be easier to fix, if that's happening.

My attempt to address this is here: https://github.com/buoyantio/linkerd/tree/ver/netty4-valed-routingfactory
I'll defer to @stevej to report on how much this helps

Our time to failure doubled but we're still full GC'ing after about 4 hours. I'll hook up YourKit and see if I can find this function in the hot path. Before, YourKit was claiming that this was like 8% of our allocations.

As I said in Slack, Alex and I didn't find evidence that it was directly helpful for reducing allocations. Considering closing this issue with a WELPWETRIED

¯_(ツ)_/¯


When doing /bus/refresh only single instance gets refreshed

When doing /bus/refresh only single instance of my client app gets refreshed where expected is all instances of my client app should get refreshed , I am using below amqp version jar
org.springframework.cloud:spring-cloud-starter-bus-amqp:1.1.2.RELEASE

We need more information.  A sample project to reproduce would be best.


A singular line of code

So I was thinking in the shower about randomize(), and thought how maybe the generalization would be boosted with interleaving episodes with the starting_representation(), like a validation. Just a thought.
That however, made me realize that reset() doesn't have access to episodes, and frankly that the environment is only having reset() called and NOT re init()!
This causes the FieldState's current_time only reaches 0 once, so the done flag is marked once and then goes towards negative infinity. So what this means is there is no problem with the GPU learning, bc the entire time only a single reward was ever propagating for every training run. I just fixed this with one line of code. Sheesh

Close this issue when there is some good training output: Variation in rew (episodic mean reward) or good behavior in the rendering


replace scripts with console_scripts entry_point

Please fix a failure of CI test.
You may need to remove bin and bin/aqt file.


Experimental support for critical-section 1.0.0-alpha

Superseded by https://github.com/rp-rs/rp-hal/pull/414


Benchmarks and IO fix

Proposed Changes
RAM consumption for loading matrices has been reduced several times.
Benchmarks on CI were not working due to NVIDIA set in config :)
Script is fixed, CI benchmarks at least work locally.
Types of changes
What types of changes does your code introduce to GraphBLAS-sharp?
Put an x in the boxes that apply

[x] Bugfix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

[x] Build and tests pass locally
[ ] I have added tests that prove my fix is effective or that my feature works (if appropriate)
[ ] I have added necessary documentation (if appropriate)

Further comments

Что-то таки с путями не то: https://github.com/YaccConstructor/GraphBLAS-sharp/actions/runs/6767965616/job/18391500043


Add ButtonStyle.premium and deprecate InteractionCallbackType.premiumRequired

Description
Implements https://github.com/discord/discord-api-docs/pull/6875
Connected issues & other potential problems
None
Type of change
Please delete options that are not relevant.

[x] New feature (non-breaking change which adds functionality)

Checklist:

[x] Ran dart analyze or make analyze and fixed all issues
[x] Ran dart format --set-exit-if-changed -l 160 ./lib or make format and fixed all issues
[x] I have performed a self-review of my own code
[ ] I have commented my code, particularly in hard-to-understand areas
[x] I have made corresponding changes to the documentation
[ ] I have added tests that prove my fix is effective or that my feature works

LGTM, will wait to see if the PR on Discord's end changes in any way before merging.


Cannot find module kind-of

encountered this problem with fresh node_modules
bundling with webpack against<EMAIL_ADDRESS>relevant issue with recent reports https://github.com/jonschlinkert/merge-deep/issues/4

looks like<EMAIL_ADDRESS>doesn't know how to parse<EMAIL_ADDRESS>WARNING in /home/xyz/Development/lavamoat/node_modules/clone-deep/utils.js 7:34-41
Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
 @ /home/xyz/Development/lavamoat/node_modules/clone-deep/index.js
 @ ../core/node_modules/merge-deep/index.js
 @ ../core/src/mergeConfig.js
 @ ./src/App.js
 @ ./src/index.js

looks like<EMAIL_ADDRESS>doesn't know how to parse<EMAIL_ADDRESS>WARNING in /home/xyz/Development/lavamoat/node_modules/clone-deep/utils.js 7:34-41
Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
 @ /home/xyz/Development/lavamoat/node_modules/clone-deep/index.js
 @ ../core/node_modules/merge-deep/index.js
 @ ../core/src/mergeConfig.js
 @ ./src/App.js
 @ ./src/index.js

due to this lazy cache mechanism https://npmfs.com/package/clone-deep/0.2.4/utils.js#L10

due to this lazy cache mechanism https://npmfs.com/package/clone-deep/0.2.4/utils.js#L10

relevant PRs https://github.com/jonschlinkert/merge-deep/pull/12/files https://github.com/jonschlinkert/merge-deep/pull/14 https://github.com/jonschlinkert/merge-deep/pull/9

relevant PRs https://github.com/jonschlinkert/merge-deep/pull/12/files https://github.com/jonschlinkert/merge-deep/pull/14 https://github.com/jonschlinkert/merge-deep/pull/9

@doowb more info over here :+1:

@doowb more info over here :+1:

@kumavis :) I just saw these... thank you for more information.
Have you tried unlazy-loader? It might be useful until we update the dependencies using lazy-cache.

@kumavis :) I just saw these... thank you for more information.
Have you tried unlazy-loader? It might be useful until we update the dependencies using lazy-cache.

fwiw just tested the latest versions and they worked with the current merge-deep source for my basic json merging needs

<EMAIL_ADDRESS><EMAIL_ADDRESS><EMAIL_ADDRESS>
not sure what breaking changes bubble up but could always just udpate them all and cut a major :shrug:

fwiw just tested the latest versions and they worked with the current merge-deep source for my basic json merging needs

<EMAIL_ADDRESS><EMAIL_ADDRESS><EMAIL_ADDRESS>
not sure what breaking changes bubble up but could always just udpate them all and cut a major :shrug:

Yes... I believe the path will be to update the dependencies and publish a new major version.
Most of the breaking changes have to do with Node.js compatibility... this is currently set at >=0.10.0, and will need to be bumped to at least what clone-deep is using (>=6) because of the use of const.

Yes... I believe the path will be to update the dependencies and publish a new major version.
Most of the breaking changes have to do with Node.js compatibility... this is currently set at >=0.10.0, and will need to be bumped to at least what clone-deep is using (>=6) because of the use of const.

also same error invoke when using puppeteer-extra-plugin

Can anyone help me with this problem?
I'm stuck.

USE assign-deep (https://www.npmjs.com/package/assign-deep) instand !

Hello!
If you're using webpack and electron I found a fix for this by making sure to include any merge-deep dependencies as an external and to declare the node_modules path relative to your package.json in your package.json as an 'extraResource'.