RFC: eio-only version of cohttp #857

bikallem · 2022-02-01T13:13:42Z

This is a WIP PR which implements an eio-only version of cohttp. Therefore the API is necessarily different as compared to other cohttp packages (lwt, async).

The HTPP protocol implemented is 1.1
It is developed against 4.12+domains switch.
It supports running the server in a multiple domains(cores) setting. It utilizes socket sharding (https://www.nginx.com/blog/socket-sharding-nginx-release-1-9-1/) so it is automatically load balanced among the cores/domains in linux. Load balancing needs to be investigated for other OSes.
Only the server API done for now. A sample server - server1.ml - in included in the PR. It can also be viewed here - https://github.com/bikallem/ocaml-cohttp/blob/eio-3/cohttp-eio/examples/server1.ml.
The server API aims to be composable. To precisely define composable, here composable means Server.middleware and Server.handler values can be defined by other external users and exist in various opam packages. And that such packages can be reused and composed together to form a web application. The demo - server1.ml - demonstrates one such use case of composing together Server.handler values. Next, Server.middleware is designed to cater to scenarios where external users can define web application functionality such as cookie based sessions, sql db based sessions, authentication and the like and a web application developer can per-use such packages. I intend to update the examples with such a demo middleware. The design of Server.handler and Server.middleware is mostly inspired by F# web library - https://github.com/giraffe-fsharp/Giraffe. However, it can also be found in another ocaml web library (https://github.com/aantron/dream).

I am opening the PR to get early feedback and to gauge maintainer appetite for this work.

Latest benchmarking results:
1 Core performance:

Multicore performance: (24 cores)

mseri · 2022-02-01T16:17:28Z

While the lwt implementation is in a delicate situation, requiring a big break with the past and the ecosystem, this one could be easily added as cohttp subpackage already now imho.

We could add an extra lower bound on 5.0.0 just for this package (leaving jsoo enabled: we know it does not yet support 5.0.0, but they can coexist, we just need to figure out the CI).

It could be interesting then to evolve this and the conduit-free implementation of lwt-server together so that they api remains consistent.

Fwiw, I think we can even leave the client out for the time being. And focus on the server to start with.

avsm · 2022-02-01T17:18:15Z

I'm in favour of @mseri's proposal. I think we can get an eio client in there too though -- it's a fairly small amount of code vs the server.

cohttp-eio/src/cohttp_eio.mli

cohttp-eio/src/parser.ml

cohttp-eio/src/cohttp_eio.mli

mseri · 2022-02-02T15:59:49Z

It looks like there is a lot of re-implementation. I think, if we call it cohttp-*, it should re-use signature from the http module (I would suggest also the parser). And, if anything needs being extended or improved, we do it at those libraries level. Ideally cohttp-eio would depend on eio and cohttp and get the relevant signatures from there, so that as much as possible of the types and the interfaces are shared.

rgrinberg · 2022-02-02T19:32:26Z

The server API aims to be composable. To precisely define composable, here composable means Server.middleware and Server.handler values can be defined by other external users and exist in various opam packages. ...

I don't think we should be introducing web framework components into cohttp. Especially since this was already implemented outside of cohttp without loss of functionality.

rgrinberg · 2022-02-02T19:33:08Z

Why did you decide to use cstructs? Anil mentioned that strings are the way to go with multicore.

rgrinberg · 2022-02-02T19:34:45Z

.envrc

@@ -0,0 +1,2 @@
+use nix shell-eio.nix 
+eval $(opam env --shell=bash --inplace-path --set-switch)


this shouldn't be committed. not everyone is using direnv.

@bikallem this file still needs to be removed

rgrinberg · 2022-02-02T19:36:14Z

cohttp-eio/src/server.ml

+    in
+    let headers =
+      let date = Unix.time () |> Unix.gmtime in
+      Http.Header.add_unless_exists headers "Date" (datetime_to_string date)


Do we really need this? I don't remember Date being mandatory.

According to this spec (https://www.rfc-editor.org/rfc/rfc7231#section-7.1.1.2), Date seems to be mandatory if the server is capable. To quote An origin server MUST send a Date header field in all other cases.

golang http lib also seems to add this header - https://github.com/golang/go/blob/master/src/net/http/server.go#L1416-L1417

That's a good point, but I'd still go against it. For a few reasons:

We don't do it in the lwt/async. It's inconsistent with respect to our own libs.

The implementation we have here is naive. Getting the current time is takes a syscall and should be cached for high performance servers.

It's just as easy to implement outside of the basic library. It could just as easily be done through middleware. Or even a simple response generation function which would work for all backends.

Agreed. We can drop Date for now and put it in the cohttp-consuming libraries

Removed now.

rgrinberg · 2022-02-02T19:41:22Z

cohttp-eio/src/server.ml

+      (*--- Don't cache set-cookie headers in browsers and proxies. ---*)
+      if Http.Header.mem headers "set-cookie" then
+        Http.Header.add headers "Cache-Control" {|no-cache="Set-Cookie"|}
+      else headers


We can just let downstream frameworks take care of this for us.

Removed now.

rgrinberg · 2022-02-02T19:41:53Z

dune-project

+;   (conf-npm :with-test)
+;   (js_of_ocaml (>= 3.3.0))
+;   (js_of_ocaml-ppx (>= 3.3.0))
+;   (js_of_ocaml-lwt (>= 3.5.0))))


Why is this commented out?

jsoo doesn't build on 4.12+domains switch.

rgrinberg · 2022-02-02T19:42:03Z

dune

@@ -0,0 +1 @@
+(data_only_dirs cohttp-lwt-jsoo cohttp_lwt_jsoo_test)


We don't need this dune file.

It is temporarily here so that dune won't build jsoo packages.

I see. Note that you could always build just the package you want with dune build cohttp-eio.install.

rgrinberg · 2022-02-02T19:42:56Z

dune-project

+  eio_main
+  cstruct
+  logs
+  (fmt (>= 0.8.7))


I could be wrong, but I don't see fmt or logs being used anywhere.

tmattio · 2022-02-02T20:59:27Z

Disclaimer: not a maintainer of Cohttp or Eio, so I'm probably missing a bit of context.

What's the reason for adding this as a new cohttp-eio package?
With effects allowing us to write APIs without monads, the use of Eio becomes an implementation detail that doesn't leak to the package interface (apart from the Reader.create function apparently, but I'm guessing that's something that could be changed). So I was expecting this would allow package maintainers to stop having to maintain multiple packages for each IO backend.

Is there something preventing us from implementing the functionality in cohttp and have the lwt and async backends "wrap" the cohttp package to keep the API compatibility?

rgrinberg · 2022-02-02T21:40:15Z

Even if we could completely drop support for 4.x, the Lwt and Async packages have their own event loops while Eio has its own. I doubt people want to use a cohttp-{lwt,async} package that brings its own event loop.

bikallem · 2022-02-02T22:04:29Z

Disclaimer: not a maintainer of Cohttp or Eio, so I'm probably missing a bit of context.

What's the reason for adding this as a new cohttp-eio package? With effects allowing us to write APIs without monads, the use of Eio becomes an implementation detail that doesn't leak to the package interface (apart from the Reader.create function apparently, but I'm guessing that's something that could be changed). So I was expecting this would allow package maintainers to stop having to maintain multiple packages for each IO backend.

Is there something preventing us from implementing the functionality in cohttp and have the lwt and async backends "wrap" the cohttp package to keep the API compatibility?

eio sits atop effects, uring and luv and has its own IO loop which works a bit differently from lwt/async. If you want direct style code in a lwt codebase, there is lwt_eio package which does that. Here is another PR which attempts to do just that (#854). Perhaps going forward if eio gains traction and becomes the "default"/blessed io package, then library authors will probably stop maintaining lwt/async packages.

bikallem · 2022-02-02T22:11:19Z

The server API aims to be composable. To precisely define composable, here composable means Server.middleware and Server.handler values can be defined by other external users and exist in various opam packages. ...

I don't think we should be introducing web framework components into cohttp. Especially since this was already implemented outside of cohttp without loss of functionality.

Agreed. It won't. cotthp-eio will provide types and maybe some glue functions to help downstream web frameworks/components provide "web framework/component" like functionalities.

tmattio · 2022-02-02T22:17:35Z

Even if we could completely drop support for 4.x, the Lwt and Async packages have their own event loops while Eio has its own. I doubt people want to use a cohttp-{lwt,async} package that brings its own event loop.

Couldn't eio offer an API to provide your own event loop? So people could use the same package but call Eio_lwt.init_event_loop () or similar to tell Eio to use Lwt (or Async's) event loop.

bikallem · 2022-02-02T22:42:50Z

Why did you decide to use cstructs? Anil mentioned that strings are the way to go with multicore.

Cstruct.t is the default/chosen data structure in eio, so we use Cstruct.t also to save the hassle of of data structure conversion/allocation/gc cost. With regards to cstruct/bigstring, AIUI Cstruct.t/bigstring allows zero copy/no allocation when data is moved from C memory space to OCaml managed memory space. Furthermore, on linux, uring removes the need to do allocation/copying when moving data from OS kernel space to user space. io_uring which eio uses has been designed to explicitly exploit these functionalities. cohttp-eio aims to also exploit these to the fullest possible. Perhaps @talex5 can elucidate/correct the above if my understanding is incorrect.

rgrinberg · 2022-02-03T01:44:15Z

cohttp-eio/src/server.ml

+      }
+  done
+
+let create ?(socket_backlog = 10_000) ?(domains = cpu_core_count) ~port


What do you think about making the default for domains to be 1? To me seems like the safest option since the user's code might not necessarily be thread safe.

Yes, it's very rare for a server to need to process requests so fast it needs more than one core for the job.

rgrinberg · 2022-02-03T01:49:14Z

Why did you decide to use cstructs? Anil mentioned that strings are the way to go with multicore.

Cstruct.t is the default/chosen data structure in eio, so we use Cstruct.t also to save the hassle of of data structure conversion/allocation/gc cost. With regards to cstruct/bigstring, AIUI Cstruct.t/bigstring allows zero copy/no allocation when data is moved from C memory space to OCaml managed memory space. Furthermore, on linux, uring removes the need to do allocation/copying when moving data from OS kernel space to user space. io_uring which eio uses has been designed to explicitly exploit these functionalities. cohttp-eio aims to also exploit these to the fullest possible. Perhaps @talex5 can elucidate/correct the above if my understanding is incorrect.

Thanks for response. This still needs some clarification. AFAIK, normal strings aren't going to be moved by the GC in 5.0 so they should be usable for io_uring and not require extra copies anyway.

rgrinberg · 2022-02-03T01:58:30Z

Agreed. It won't. cotthp-eio will provide types and maybe some glue functions to help downstream web frameworks/components provide "web framework/component" like functionalities.

In which case, we should drop the middleware alias. You have a noble goal in mind, but I doubt it will have the outcome we all desire. Who is to say that the various OCaml web frameworks popular today are going to use this type? I don't yet see any interest from them their maintainers, therefore I doubt that this is going to be adopted. To get some unification here, we need to get the various maintainers on board first and only then start introducing stuff to smooth out compatibility. Otherwise, we're just going to introduce the nth+1 framework.

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri · 2022-06-08T19:12:24Z

cohttp-eio/src/reader.ml

+      let headers = http_headers t in
+      let encoding = Http.Header.get_transfer_encoding headers in
+      commit t;
+      { Http.Request.meth; resource; version; headers; scheme = None; encoding }


Since we were talking about scheme, why here we set it to None? We don't have a way to populate it?
Is the warning due to the use of encoding?

Since we were talking about scheme, why here we set it to None? We don't have a way to populate it?

I am not sure if there is a reliable way to determine a value for the scheme. For example, a web app using cohttp-eio could be behind a proxy server where the ssl/tls terminate or it could be a normal socket connection without ssl/tls.

Is the warning due to the use of encoding?

Yes, dune gives a build error during dev without the attribute.

mseri · 2022-06-08T19:13:21Z

I have cleaned the commits from the spurious changes inherited with the merges. Just had a final question, for the rest I am waiting for the CI to make sure I did not screw up anything

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri · 2022-06-08T21:13:14Z

The tests on nix are failing with

File "cohttp-eio/src/dune", line 4, characters 12-15:
4 |  (libraries eio cstruct http bigstringaf fmt))
                ^^^
Error: Library "eio" not found.
Hint: try:
  dune external-lib-deps --missing -j 1 @runtest
File "cohttp-eio/tests/dune", line 6, characters 23-31:
6 |  (libraries cohttp_eio eio_main fmt))
                           ^^^^^^^^
Error: Library "eio_main" not found.
Hint: try:
  dune external-lib-deps --missing -j 1 @runtest
File "cohttp-eio/tests/dune", line 13, characters 23-31:
13 |  (libraries cohttp_eio eio_main fmt))
                            ^^^^^^^^
Error: Library "eio_main" not found.
Hint: try:
  dune external-lib-deps --missing -j 1 @runtest
File "cohttp-eio/tests/dune", line 22, characters 0-50:
22 | (mdx
23 |  (package cohttp-eio)
24 |  (packages cohttp-eio))
Error: Program ocaml-mdx not found in the tree or in PATH
 (context: default)
Hint: opam install mdx
File "cohttp-eio/tests/dune", line 27, characters 9-25:
27 |  (deps %{bin:test-server} %{bin:test-chunk-server} %{bin:crlf}))
              ^^^^^^^^^^^^^^^^
Error: Program test-server not found in the tree or in PATH
 (context: default)

bikallem · 2022-06-09T08:22:53Z

I am not familiar with nix but I think nix needs to not run the cohttp-eio package and tests for now. If nix is to run on cohttp-eio then it needs to run a version of OCaml with effects and domains, 4.12.0+domains or 5.0.0+trunk. @rgrinberg

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

They are broken on the CI environments for various infrastructural reasons Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

bikallem

shell-eio.nix only installs/provides system dependencies when working in nixos. It will not setup the required OCaml version (4.12.+domains or 5.0.0). For that I believe it needs a nix package to be submitted to nixpkgs repo in github. I am not familiar enough with nixpkgs to be able to package trunk OCaml. Best leave the eio part out of nix.yml for now.

internal/modules/cjs/loader.js:905 throw err; ^ Error: Cannot find module 'xmlhttprequest' Fixing this requires knowing npm, yarn and node, so I will disable them for the moment Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

We need a nix user to know how to install ocaml 5.0.0+trunk on nix Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri · 2022-06-09T10:12:04Z

The last two commits should fix the CI, if also nix is green I will merge

mseri · 2022-06-09T12:15:50Z

It is all green now. Time to take a stance a merge. The next cohttp release will be a alpha o beta release anyway, so extra changes will have time to be made

mseri · 2022-06-09T12:16:04Z

Thanks a lot for the patience and the effort

…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha0) CHANGES: - cohttp-eio: ensure "Host" header is the first header in http client requests (bikallem mirage/ocaml-cohttp#939) - cohttp-eio: add TE header in client. Check TE header is server (bikallem mirage/ocaml-cohttp#941) - cohttp-eio: add User-Agent header to request from Client (bikallem mirage/ocaml-cohttp#940) - cohttp-eio: add Content-Length header to request/response (bikallem mirage/ocaml-cohttp#929) - cohttp-eio: add cohttp-eio client api - Cohttp_eio.Client (bikallem mirage/ocaml-cohttp#879) - http: add requires_content_length function for requests and responses (bikallem mirage/ocaml-cohttp#879) - cohttp-eio: use Eio.Buf_write and improve server API (talex5 mirage/ocaml-cohttp#887) - cohttp-eio: update to Eio 0.3 (talex5 mirage/ocaml-cohttp#886) - cohttp-eio: convert to Eio.Buf_read (talex5 mirage/ocaml-cohttp#882) - cohttp lwt client: Connection cache and explicit pipelining (madroach mirage/ocaml-cohttp#853) - http: add Http.Request.make and simplify Http.Response.make (bikallem mseri mirage/ocaml-cohttp#878) - http: add pretty printer functions (bikallem mirage/ocaml-cohttp#880) - New eio based client and server on top of the http library (bikallem mirage/ocaml-cohttp#857) - New curl based clients (rgrinberg mirage/ocaml-cohttp#813) + cohttp-curl-lwt for an Lwt backend + cohttp-curl-async for an Async backend - Completely new Parsing layers for servers (anuragsoni mirage/ocaml-cohttp#819) + Cohttp now uses an optimized parser for requests. + The new parser produces much less temporary buffers during read operations in servers. - Faster header comparison (gasche mirage/ocaml-cohttp#818) - Introduce http package containing common signatures and structures useful for compatibility with cohttp - and no dependencies (rgrinberg mirage/ocaml-cohttp#812) - async(server): allow reading number of active connections (anuragsoni mirage/ocaml-cohttp#809) - Various internal refactors (rgrinberg, mseri, mirage/ocaml-cohttp#802, mirage/ocaml-cohttp#812, mirage/ocaml-cohttp#820, mirage/ocaml-cohttp#800, mirage/ocaml-cohttp#799, mirage/ocaml-cohttp#797) - http (all cohttp server backends): Consider the connection header in response in addition to the request when deciding on whether to keep a connection alive (anuragsoni, mirage/ocaml-cohttp#843) + The user provided Response can contain a connection header. That header will also be considered in addition to the connection header in requests when deciding whether to use keep-alive. This allows a handler to decide to close a connection even if the client requested a keep-alive in the request. - async(server): allow creating a server without using conduit (anuragsoni mirage/ocaml-cohttp#839) + Add `Cohttp_async.Server.Expert.create` and `Cohttp_async.Server.Expert.create_with_response_action`that can be used to create a server without going through Conduit. This allows creating an async TCP server using the Tcp module from `Async_unix` and lets the user have more control over how the `Reader.t` and `Writer.t` are created. - http(header): faster `to_lines` and `to_frames` implementation (mseri mirage/ocaml-cohttp#847) - cohttp(cookies): use case-insensitive comparison to check for `set-cookies` (mseri mirage/ocaml-cohttp#858) - New lwt based server implementation: cohttp-server-lwt-unix + This new implementation does not depend on conduit and has a simpler and more flexible API - async: Adapt cohttp-curl-async to work with core_unix. - *Breaking changes* + refactor: move opam metadata to dune-project (rgrinberg mirage/ocaml-cohttp#811) + refactor: deprecate Cohttp_async.Io (rgrinberg mirage/ocaml-cohttp#807) + fix: move more internals to Private (rgrinberg mirage/ocaml-cohttp#806) + fix: deprecate transfer encoding field (rgrinberg mirage/ocaml-cohttp#805) + refactor: deprecate Cohttp_async.Body_raw (rgrinberg mirage/ocaml-cohttp#804) + fix: deprecate more aliases (rgrinberg mirage/ocaml-cohttp#803) + refactor: deprecate connection value(rgrinberg mirage/ocaml-cohttp#798) + refactor: deprecate using attributes (rgrinberg mirage/ocaml-cohttp#796) + cleanup: remove cohttp-{curl,server}-async (rgrinberg mirage/ocaml-cohttp#904) + cleanup: remove cohttp-{curl,server,proxy}-lwt (rgrinberg mirage/ocaml-cohttp#904) + fix: all parsers now follow the spec and require `\r\n` endings. Previously, the `\r` was optional. (rgrinberg, mirage/ocaml-cohttp#921) - `cohttp-lwt-jsoo`: do not instantiate `XMLHttpRequest` object on boot (mefyl mirage/ocaml-cohttp#922)

bikallem mentioned this pull request Feb 1, 2022

RFC : Port cohttp-lwt-unix to eio using lwt_eio #854

Closed

bikallem changed the title ~~RFC: pure eio version of cohttp~~ RFC: eio-only version of cohttp Feb 1, 2022

bikallem force-pushed the eio-3 branch from 2350a7e to 82197b3 Compare February 2, 2022 15:48

mseri reviewed Feb 2, 2022

View reviewed changes

cohttp-eio/src/cohttp_eio.mli Outdated Show resolved Hide resolved

mseri reviewed Feb 2, 2022

View reviewed changes

cohttp-eio/src/parser.ml Outdated Show resolved Hide resolved

mseri reviewed Feb 2, 2022

View reviewed changes

cohttp-eio/src/cohttp_eio.mli Outdated Show resolved Hide resolved

bikallem force-pushed the eio-3 branch 4 times, most recently from 1f993c6 to f42f787 Compare February 2, 2022 18:30

rgrinberg reviewed Feb 2, 2022

View reviewed changes

rgrinberg reviewed Feb 3, 2022

View reviewed changes

bikallem and others added 3 commits June 8, 2022 21:08

cohttp-eio(ci): implement ci using ocaml/setup-ocaml

82d7d9f

cohtto-eio: refactor common client and server functionality

81539d0

Update CHANGES

72d6648

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri force-pushed the eio-3 branch from 29d4cf8 to 72d6648 Compare June 8, 2022 19:08

mseri reviewed Jun 8, 2022

View reviewed changes

ci: update use of alpha repository for ocaml 5.0.0

c444f76

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri force-pushed the eio-3 branch from 6313ea3 to c444f76 Compare June 8, 2022 19:26

nix: runtest -j 1, somehow GH ci is having issues with more

21f31f7

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri added 3 commits June 9, 2022 10:28

ci: dont try to run eio tests in nix

36927d3

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

ci: disable js-tests

6afe32c

They are broken on the CI environments for various infrastructural reasons Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

ci: test eio and js with nix

e65f724

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

bikallem commented Jun 9, 2022

View reviewed changes

mseri added 2 commits June 9, 2022 12:08

ci: js-tests are broken also on nix

4158050

internal/modules/cjs/loader.js:905 throw err; ^ Error: Cannot find module 'xmlhttprequest' Fixing this requires knowing npm, yarn and node, so I will disable them for the moment Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

ci: don't test cohttp-eio on nix

2c436df

We need a nix user to know how to install ocaml 5.0.0+trunk on nix Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri merged commit 1dc4746 into mirage:master Jun 9, 2022

mseri mentioned this pull request Oct 24, 2022

[new release] http, cohttp, cohttp-top, cohttp-server-lwt-unix, cohttp-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async and cohttp-async (6.0.0~alpha0) ocaml/opam-repository#22353

Merged

bikallem deleted the eio-3 branch November 9, 2022 14:24

		@@ -0,0 +1,2 @@
		use nix shell-eio.nix
		eval $(opam env --shell=bash --inplace-path --set-switch)

		@@ -0,0 +1 @@
		(data_only_dirs cohttp-lwt-jsoo cohttp_lwt_jsoo_test)

RFC: eio-only version of cohttp #857

RFC: eio-only version of cohttp #857

Conversation

bikallem commented Feb 1, 2022 • edited Loading

mseri commented Feb 1, 2022 • edited Loading

avsm commented Feb 1, 2022

mseri commented Feb 2, 2022

rgrinberg commented Feb 2, 2022

rgrinberg commented Feb 2, 2022

rgrinberg Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bikallem Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmattio commented Feb 2, 2022

rgrinberg commented Feb 2, 2022 • edited Loading

bikallem commented Feb 2, 2022

bikallem commented Feb 2, 2022

tmattio commented Feb 2, 2022

bikallem commented Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Feb 3, 2022

rgrinberg commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseri commented Jun 8, 2022

mseri commented Jun 8, 2022 • edited Loading

bikallem commented Jun 9, 2022

bikallem left a comment

Choose a reason for hiding this comment

mseri commented Jun 9, 2022

mseri commented Jun 9, 2022

mseri commented Jun 9, 2022

bikallem commented Feb 1, 2022 •

edited

Loading

mseri commented Feb 1, 2022 •

edited

Loading

rgrinberg Feb 2, 2022 •

edited

Loading

bikallem Feb 2, 2022 •

edited

Loading

rgrinberg commented Feb 2, 2022 •

edited

Loading

bikallem commented Feb 2, 2022 •

edited

Loading

mseri commented Jun 8, 2022 •

edited

Loading