Serialize Cursors #81

sambostock · 2021-04-21T00:17:18Z

TL;DR

This is a redo of #73, but using a deprecation warning instead of raising an error. The error is deferred until 2.0.

Details

It is unsafe to use cursors which cannot be serialized using JSON.dump and deserialized again using JSON.parse. Such cursors may result in a different cursor being deserialized, or in serialization failing entirely, which may result in incorrect behaviour in iterative jobs.

Starting in 2.0, usage of such cursors will raise an ArgumentError. Until then, a deprecation warning will be logged.

#73 contains more context on the change, and #80 contains information as to the motivation for this alternative approach.

To facilitate migration, this also adds

a global enforce_serializable_cursors config which can force the error 2.0 will introduce
- a per-job-class enforce_serializable_cursors inheritable override config

⚠️ Before Merging

Rebase after Add headings & formatting to Custom Enumerator guide #439 is merged.
Rebase after Add JobIteration::Deprecation #441 is merged.

Closes #80

sambostock · 2021-04-21T00:21:18Z

lib/job-iteration.rb

@@ -9,6 +9,8 @@ module JobIteration

  INTEGRATIONS = [:resque, :sidekiq]

+  Deprecation = ActiveSupport::Deprecation.new("2.0", "JobIteration")


I originally used ActiveSupport::Deprecation.warn directly, that labelled deprecations as coming from Rails. This uses our own object, but does mean that the host app's settings for ActiveSupport::Deprecation do not propagate (e.g. disallowed_deprecations).

lib/job-iteration/iteration.rb

sambostock · 2021-04-21T00:25:17Z

test/integration/integration_behaviour.rb

@@ -35,7 +35,7 @@ module IntegrationBehaviour
    end

    test "unserializable corruption is prevented" do
-      skip "Deferred until 2.0.0"
+      skip "Breaking change deferred until 2.0" if Gem::Version.new(JobIteration::VERSION) < Gem::Version.new("2.0")


I put this in to ensure the change makes it into 2.0

sambostock · 2021-04-21T00:25:46Z

test/unit/iteration_test.rb

@@ -66,6 +66,7 @@ def each_iteration(*)
  class InvalidCursorJob < ActiveJob::Base
    include JobIteration::Iteration
    def each_iteration(*)
+      return if Gem::Version.new(JobIteration::VERSION) < Gem::Version.new("2.0")


Likewise here. We can't bump to 2.0 without also making unsafe cursors an error.

test/unit/iteration_test.rb

sambostock · 2021-04-21T00:29:02Z

lib/job-iteration/iteration.rb

-        "Strings, Integers, Floats, Arrays, Hashes, true, false, or nil.",
-        cursor: cursor,
-      )
+      Deprecation.warn(<<~DEPRECATION_MESSAGE.chomp)


I originally looked at using deprecation_warning (as mentioned here), which has a much richer built in message. However, it is method oriented, and we aren't getting rid of any method.

GustavoCaso · 2021-04-21T16:51:52Z

lib/job-iteration/iteration.rb

-      )
+      Deprecation.warn(<<~DEPRECATION_MESSAGE.chomp)
+        Cursor must be composed of objects capable of built-in (de)serialization:
+          Strings, Integers, Floats, Arrays, Hashes, true, false, or nil.


Should we also allow to pass Symbol as part of the cursor?

Yesterday we noticed that having a Hash with keys as symbols also broke. Hash with symbols should be accepted as well 😄

No. Symbols are not safe to serialize as JSON, because they are deserialized as Strings:

$ ruby -rjson -e 'p JSON.parse(JSON.dump(:symbol))' "symbol"

cursor_position is serialized by the queue adapter (Resque/Sidekiq), not by ActiveJob itself, so none of Rails' magic symbol serialization happens.

Thanks for the explanation

GustavoCaso · 2021-04-21T16:56:03Z

test/unit/iteration_test.rb

-      error.message,
-    )
+  def assert_cursor_deprecation_warning
+    original_behaviour = JobIteration::Deprecation.behavior


Could we simplify this test assertion with something like:

JobIteration::Deprecation.any_instance.expects(:warn)

Feels that the current implementation is quite complex

We could, I just wanted to make sure the deprecation message was formatted correctly.

sambostock · 2021-04-21T18:28:09Z

I'll simplify the deprecation warning message 👍

sambostock · 2021-06-29T22:09:54Z

I've updated the description to be simpler, and slightly tweaked the test helpers. Should be good for final review.

CI on Ruby 2.5 will start passing again once sorbet/sorbet#4281 is merged and a new version of sorbet-runtime is released; it is not related to these changes.

sambostock · 2022-07-01T06:04:53Z

I've circled back to this and made some changes. This PR still introduces the deprecation warning, however it also introduces a config to force the new behavior. This config can be set globally and/or per-job, allowing consumers to globally opt-in to enforcement ahead of 2.0, and optionally opt-out per job until they are made compliant, or alternatively opt-in per compliant job until they are able to opt-in globally.

This should allow for a smoother transition. Version 2.0 would simply delete (or no-op & deprecate) the config and instruct consumers to remove it from their code.

I've also added documentation about this, which applies regardless of the deprecation/error.

lib/job-iteration.rb

guides/custom-enumerator.md

bdewater · 2022-07-12T00:02:19Z

lib/job-iteration.rb

@@ -20,6 +22,25 @@ module JobIteration
  # Defaults to nil which means that jobs will not be interrupted except on termination signal.
  attr_accessor :max_job_runtime

+  # Set this to `true` to enforce that cursors be composed of objects capable
+  # of built-in (de)serialization: Strings, Integers, Floats, Arrays, Hashes,
+  # true, false, or nil.


Does it have to be only these built-in primitives? Active Job allows custom serializers to be defined for parameters, like we use in our Money gem. Would be nice if we can piggyback on top of this.

Hmm... Good question. If Active Job performs a serialization step before handing off the job to the backend, and if we can test that an object is serializable by it, then that should work. I shall have to look into this.

Indeed my goal is not to be restrictive, but to prevent foot-guns, as my team encountered.

Okay, so with the code as-is, no, we couldn't rely on this behavior, as Active Job only serializes arguments, and doesn't bother with the rest of the job data.

However, an alternative approach might be to serialize and deserialize the cursor ourselves, since we do already override those methods:

job-iteration/lib/job-iteration/iteration.rb

Lines 79 to 92 in 17e6ec5

def serialize # @private

super.merge(

"cursor_position" => cursor_position,

"times_interrupted" => times_interrupted,

"total_time" => total_time,

)

end

def deserialize(job_data) # @private

super

self.cursor_position = job_data["cursor_position"]

self.times_interrupted = job_data["times_interrupted"] || 0

self.total_time = job_data["total_time"] || 0

end

It's a bit awkward because the serialization API expects an Array of arguments, but we could conceivably do something like

def serialize # @private super.merge( "cursor_position" => ActiveJob::Arguments.serialize([cursor_position]).first, "times_interrupted" => times_interrupted, "total_time" => total_time, ) end def deserialize(job_data) # @private super self.cursor_position = ActiveJob::Arguments.deserialize([job_data["cursor_position"]]).first self.times_interrupted = job_data["times_interrupted"] || 0 self.total_time = job_data["total_time"] || 0 end

Another consideration is that the enforcement API here happens on every cursor, regardless of if the job is interrupted, which has pros and cons compared to the serialization approach:

Checking every cursor involves more computations than only serializing as needed (and blowing up if it fails)

Checking every cursor gives you instant feedback immediately after the enumerator produces an unserializable cursor (usually before the first call to each_iteration), rather than only if the job is interrupted and the cursor requires serialization

I suppose we could keep cursor serializability enforced for each cursor by simply attempting to serialize it, but I'm not sure how much more expensive that is compared to the naive class checking approach.

I've opened #252 branching off this PR to explore this idea, and it seems to work nicely. @Mangara, @bdewater any thoughts on it?

sambostock · 2022-07-19T06:08:54Z

Alright, I've updated the branch again, this time to leverage ActiveJob::Arguments to support cursors of any type it supports.

sambostock · 2022-07-19T06:09:50Z

CHANGELOG.md

@@ -2,6 +2,8 @@

 - [241](https://github.com/Shopify/job-iteration/pull/241) - Require Ruby 2.7+, dropping 2.6 support
 - [241](https://github.com/Shopify/job-iteration/pull/241) - Require Rails 6.0+, dropping 5.2 support
+- [80](https://github.com/Shopify/job-iteration/pull/80) - Deprecate un(de)serializable cursors


Should probably update this to "Serialize cursors"

sambostock · 2022-07-19T06:11:06Z

lib/job-iteration/iteration.rb

+    def serialize_cursor(cursor)
+      ActiveJob::Arguments.serialize([cursor]).first
+    rescue ActiveJob::SerializationError
+      raise if job_iteration_enforce_serializable_cursors


This used to wrap the error in a custom CursorError, but I figure it might be adequate to just re-raise the ActiveJob::SerializationError.

lib/job-iteration/iteration.rb

sambostock · 2022-07-19T06:13:13Z

test/support/jobs.rb

  include JobIteration::Iteration
+  UnserializableCursor = Class.new


Since we're getting all of ActiveJob's serialization now, we have to explicitly create an object that cannot be serialized. BasicObject doesn't work though, because that straight up blows up ActiveJob because it doesn't respond to respond_to? 😅

sambostock · 2022-07-19T06:13:44Z

test/unit/iteration_test.rb

+  UnserializableCursor = Class.new
+  SerializableCursor = Class.new
+
+  class SerializableCursorSerializer < ActiveJob::Serializers::ObjectSerializer


Alternatively, I could make it work with GlobalID.

sambostock · 2022-07-19T17:22:53Z

lib/job-iteration/iteration.rb

+      if job_data.key?("serialized_cursor_position")
+        deserialize_cursor(job_data.fetch("serialized_cursor_position"))


I've updated this so if a serialized cursor is found, it should always be deserializable.

sambostock · 2022-07-19T17:25:17Z

test/unit/iteration_test.rb

+  def test_job_interrupted_with_only_cursor_position_should_resume
+    with_interruption do
+      InfiniteCursorLoggingJob.perform_now
+
+      work_one_job do |job_data|
+        job_data["cursor_position"] = "raw cursor"
+        job_data.delete("serialized_cursor_position")
+      end
+
+      assert_equal("raw cursor", InfiniteCursorLoggingJob.cursors.last)
+    end
+  ensure
+    InfiniteCursorLoggingJob.cursors.clear
+  end
+
+  def test_job_interrupted_with_serialized_cursor_position_should_ignore_unserialized_cursor_position
+    with_interruption do
+      InfiniteCursorLoggingJob.perform_now
+
+      work_one_job do |job_data|
+        job_data["cursor_position"] = "should be ignored"
+        job_data["serialized_cursor_position"] = ActiveJob::Arguments.serialize([SerializableCursor.new]).first
+      end
+
+      assert_instance_of(SerializableCursor, InfiniteCursorLoggingJob.cursors.last)
+    end
+  ensure
+    InfiniteCursorLoggingJob.cursors.clear
+  end


I'm not sure if I'm missing something, but I wasn't able to find any tests that actually exercise resuming jobs. Everything seems to be testing running or interrupting jobs, but no tests actually resume anything. So this happens to add test coverage for that 😅

bdewater

Some suggestions to polish things up, but overall this looks good!

guides/custom-enumerator.md

lib/job-iteration/iteration.rb

guides/custom-enumerator.md

sambostock · 2022-10-29T03:42:07Z

Sidekiq 7 has been released, and it now errors if job arguments contain objects that don't round trip as JSON, so this PR is now not only desirable to avoid bugs, but also improves compatibility with Sidekiq.

I'll see if I can get it over the finish line this week.

sambostock · 2022-11-01T06:29:38Z

Looks like CI is broken for Rails edge due to the removal of ActiveRecord::SchemaMigration.table_name, as described in
DatabaseCleaner/database_cleaner-active_record#83.

adrianna-chang-shopify

Nice work on this Sam, I know it's been a long time coming! 🙌

A couple minor nits / comments, but nothing blocking.

guides/custom-enumerator.md

adrianna-chang-shopify · 2022-11-09T14:08:13Z

test/unit/iteration_test.rb

+    end
+  end
+
+  def test_jobs_using_unserializable_cursor_when_interrupted_should_only_store_the_cursor_and_no_serialized_cursor


I love the thoroughness of your tests, but I'm wondering about the value of this test (and the similar one below). Technically how the cursors are stored is private to JobIteration::Iteration. IMHO it feels like we're testing the implementation too much here vs the expected behaviour.

I think that testing whether we can interrupt + resume jobs that are using unserializable cursors is covered by test_job_interrupted_with_only_cursor_position_should_resume.

I wonder if test_jobs_using_serializable_cursor_when_interrupted_should_store_both_legacy_cursor_and_serialized_cursor and test_job_interrupted_with_serialized_cursor_position_should_ignore_unserialized_cursor_position could be combined into one test that , rather than explicitly checking how the cursors are being stored and read, simply checks that a job can be interrupted + resumed with a serializable cursor as expected?

Let me know if that makes sense 😅

I's been a while since I wrote these, but IIRC the reason for testing this in so much detail is that we want to ensure dequeuing works across gem versions. During the upgrade (or downgrade), jobs may be:

interrupted on an old replica, and

resumed on an old replica, or

resumed on an new replica, or

interrupted on a new replica, and

resumed on a new replica, or

resumed on an old replica

Because we can't actually test those scenarios (without significant effort), we test:

what happens given various inputs arguments we might encounter in such scenarios, and

what the output arguments are in such scenarios.

Yeah, skimming through these tests now, they make sense to me. I probably just wasn't thinking about needing to make sure we test forwards / backwards compatibility with job workers running the new vs old version of the code 🤷‍♀️

We should be able to remove some of these test cases for the 2.0 release that enforces serializable cursors, right?

Yeah. 2.0 should be able to safely stop reading (and writing) to the old argument, and we can (at least) drop all the tests that use it as an input/output, if not drop all the tests on the implementation and just test the resumption behavior.

test/unit/iteration_test.rb

sambostock

Rebased on latest main, and made some small changes, the main one being that we're defining the instance reader for job_iteration_enforce_serializable_cursors ourselves, to use the per-class setting if non-nil?, and otherwise fallback to the global setting. @Mangara and I 🎩'd on a test app and can confirm the deprecation warning shows up, and enabling serializable cursor enforcement works.

sambostock · 2023-11-19T21:35:13Z

test/unit/iteration_test.rb

+    end
+  end
+
+  def test_jobs_using_unserializable_cursor_when_interrupted_should_only_store_the_cursor_and_no_serialized_cursor


I's been a while since I wrote these, but IIRC the reason for testing this in so much detail is that we want to ensure dequeuing works across gem versions. During the upgrade (or downgrade), jobs may be:

interrupted on an old replica, and

resumed on an old replica, or

resumed on an new replica, or

interrupted on a new replica, and

resumed on a new replica, or

resumed on an old replica

Because we can't actually test those scenarios (without significant effort), we test:

what happens given various inputs arguments we might encounter in such scenarios, and

what the output arguments are in such scenarios.

guides/custom-enumerator.md

lib/job-iteration/iteration.rb

Mangara · 2023-11-20T20:51:49Z

lib/job-iteration/iteration.rb

@@ -286,6 +281,13 @@ def job_iteration_max_job_runtime
      [global_max, class_max].min
    end

+    def job_iteration_enforce_serializable_cursors? # TODO: Add a test for the edge case of registering it afterwards


Did we add this test?

Good catch – yes, test_global_max_job_runtime_with_updated_value

Then we should remove the TODO 🙂

sambostock · 2023-11-21T04:08:23Z

lib/job-iteration/iteration.rb

+
+      Deprecation.warn(<<~DEPRECATION_MESSAGE, caller_locations(3))
+        The Enumerator returned by #{self.class.name}#build_enumerator yielded a cursor which is unsafe to serialize.
+        See https://github.com/Shopify/job-iteration/blob/main/guides/custom-enumerator.md#cursor-types


This needs to match the heading we added.

Suggested change

See https://github.com/Shopify/job-iteration/blob/main/guides/custom-enumerator.md#cursor-types

See https://github.com/Shopify/job-iteration/blob/main/guides/custom-enumerator.md#cursor-serialization

Previously, `cursor_position` was handed as-is to the queue adapter. This could lead to the queue adapter corrupting cursors of certain classes. For example, if given a `Time` cursor, Sidekiq would save it as JSON by calling `to_s`, resulting in the deserialized cursor being a `String` instead of a `Time`. To prevent this, we now leverage `ActiveJob::Arguments` to (de)serialize the `cursor_position` and ensure it will make the round trip safely. However, as this is a breaking change (as unsafe cursors would previously be accepted, but possibly corrupted, whereas they would now be rejected), we begin by rescuing (de)serialization failures and emitting a deprecation warning. Starting in Job Iteration version 2.0, the deprecation warning will be removed, and (de)serialization failure will raise. Application owners can opt-in to the 2.0 behavior either globally by setting JobIteration.enforce_serializable_cursors = true or on an inheritable per-class basis by setting class MyJob < ActiveJob::Base include JobIteration::Iteration self.job_iteration_enforce_serializable_cursors = true # ... end

adrianna-chang-shopify · 2023-11-21T18:56:29Z

test/unit/iteration_test.rb

+    end
+  end
+
+  def test_jobs_using_unserializable_cursor_when_interrupted_should_only_store_the_cursor_and_no_serialized_cursor


Yeah, skimming through these tests now, they make sense to me. I probably just wasn't thinking about needing to make sure we test forwards / backwards compatibility with job workers running the new vs old version of the code 🤷‍♀️

We should be able to remove some of these test cases for the 2.0 release that enforces serializable cursors, right?

sambostock added the enhancement New feature or request label Apr 21, 2021

sambostock requested review from rafaelfranca, kirs and GustavoCaso April 21, 2021 00:17

sambostock self-assigned this Apr 21, 2021

sambostock commented Apr 21, 2021

View reviewed changes

GustavoCaso reviewed Apr 21, 2021

View reviewed changes

sambostock force-pushed the deprecate-unsafe-cursors branch 2 times, most recently from aed4b36 to 123ac38 Compare June 29, 2021 22:07

sambostock requested a review from GustavoCaso June 29, 2021 22:08

sambostock force-pushed the deprecate-unsafe-cursors branch from 123ac38 to c148ad4 Compare August 27, 2021 15:15

Mangara approved these changes Aug 27, 2021

View reviewed changes

sambostock force-pushed the deprecate-unsafe-cursors branch from c148ad4 to f8eb4f4 Compare July 1, 2022 06:01

sambostock force-pushed the deprecate-unsafe-cursors branch from f8eb4f4 to 33112f0 Compare July 4, 2022 16:01

sambostock requested review from Mangara and adrianna-chang-shopify July 4, 2022 16:02

sambostock commented Jul 4, 2022

View reviewed changes

lib/job-iteration.rb Outdated Show resolved Hide resolved

sambostock mentioned this pull request Jul 5, 2022

Enable per job class max_job_runtime #240

Merged

1 task

Mangara approved these changes Jul 6, 2022

View reviewed changes

guides/custom-enumerator.md Outdated Show resolved Hide resolved

bdewater reviewed Jul 12, 2022

View reviewed changes

sambostock mentioned this pull request Jul 12, 2022

Investigate using ActiveJob::Serializers to serialize cursor #252

Closed

sambostock force-pushed the deprecate-unsafe-cursors branch from 8186aad to eb2859f Compare July 19, 2022 06:04

sambostock requested review from Mangara and bdewater July 19, 2022 06:05

sambostock changed the title ~~Deprecate usage of cursors not safe to serialize~~ Serialize Cursors Jul 19, 2022

sambostock commented Jul 19, 2022

View reviewed changes

sambostock force-pushed the deprecate-unsafe-cursors branch from eb2859f to e96553e Compare July 19, 2022 17:19

sambostock commented Jul 19, 2022

View reviewed changes

bdewater approved these changes Jul 21, 2022

View reviewed changes

guides/custom-enumerator.md Outdated Show resolved Hide resolved

guides/custom-enumerator.md Outdated Show resolved Hide resolved

lib/job-iteration/iteration.rb Outdated Show resolved Hide resolved

guides/custom-enumerator.md Outdated Show resolved Hide resolved

sambostock force-pushed the deprecate-unsafe-cursors branch from e96553e to 6fb5f5e Compare November 1, 2022 06:18

bdewater mentioned this pull request Nov 1, 2022

Replace DatabaseCleaner with Active Record to truncate tables #294

Merged

sambostock force-pushed the deprecate-unsafe-cursors branch 2 times, most recently from 39af58a to efd94dd Compare November 9, 2022 02:53

adrianna-chang-shopify approved these changes Nov 9, 2022

View reviewed changes

sambostock force-pushed the deprecate-unsafe-cursors branch from 1c662e2 to c62500c Compare February 14, 2023 17:01

sambostock mentioned this pull request Feb 14, 2023

Continue other jobs on Rails edge failures #339

Merged

sambostock force-pushed the deprecate-unsafe-cursors branch from c62500c to 07b6fd3 Compare February 16, 2023 15:56

sambostock force-pushed the deprecate-unsafe-cursors branch from 07b6fd3 to c62500c Compare August 31, 2023 18:43

Mangara mentioned this pull request Sep 5, 2023

Revert "Revert all changes to main from v1.4.0" #428

Open

sambostock mentioned this pull request Nov 19, 2023

Add headings & formatting to Custom Enumerator guide #439

Merged

sambostock force-pushed the deprecate-unsafe-cursors branch from c62500c to e4356ed Compare November 19, 2023 21:24

sambostock commented Nov 19, 2023

View reviewed changes

sambostock force-pushed the deprecate-unsafe-cursors branch 2 times, most recently from 38e4cc1 to f39525d Compare November 20, 2023 18:31

Mangara approved these changes Nov 20, 2023

View reviewed changes

sambostock force-pushed the deprecate-unsafe-cursors branch from f39525d to a434fe8 Compare November 20, 2023 21:41

sambostock commented Nov 21, 2023

View reviewed changes

sambostock added 2 commits November 21, 2023 03:35

Add Railtie

5dad6a3

Add Deprecation

cbb89f8

sambostock mentioned this pull request Nov 21, 2023

Add JobIteration::Deprecation #441

Merged

sambostock force-pushed the deprecate-unsafe-cursors branch from a434fe8 to 5086f6d Compare November 21, 2023 08:49

adrianna-chang-shopify approved these changes Nov 21, 2023

View reviewed changes

		@@ -9,6 +9,8 @@ module JobIteration

		INTEGRATIONS = [:resque, :sidekiq]

		Deprecation = ActiveSupport::Deprecation.new("2.0", "JobIteration")

	def serialize # @private
	super.merge(
	"cursor_position" => cursor_position,
	"times_interrupted" => times_interrupted,
	"total_time" => total_time,
	)
	end

	def deserialize(job_data) # @private
	super
	self.cursor_position = job_data["cursor_position"]
	self.times_interrupted = job_data["times_interrupted"] \|\| 0
	self.total_time = job_data["total_time"] \|\| 0
	end

		include JobIteration::Iteration
		UnserializableCursor = Class.new

		if job_data.key?("serialized_cursor_position")
		deserialize_cursor(job_data.fetch("serialized_cursor_position"))

	See https://github.com/Shopify/job-iteration/blob/main/guides/custom-enumerator.md#cursor-types
	See https://github.com/Shopify/job-iteration/blob/main/guides/custom-enumerator.md#cursor-serialization

Serialize Cursors #81

Are you sure you want to change the base?

Serialize Cursors #81

Conversation

sambostock commented Apr 21, 2021 • edited Loading

TL;DR

Details

⚠️ Before Merging

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambostock Apr 21, 2021 • 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

GustavoCaso Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambostock commented Apr 21, 2021

sambostock commented Jun 29, 2021

sambostock commented Jul 1, 2022 • edited Loading

bdewater Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambostock Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambostock commented Jul 19, 2022

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

bdewater left a comment

Choose a reason for hiding this comment

sambostock commented Oct 29, 2022

sambostock commented Nov 1, 2022

adrianna-chang-shopify left a 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

sambostock left a 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

sambostock commented Apr 21, 2021 •

edited

Loading

sambostock Apr 21, 2021 •

edited

Loading

GustavoCaso Apr 21, 2021 •

edited

Loading

sambostock commented Jul 1, 2022 •

edited

Loading

bdewater Jul 12, 2022 •

edited

Loading

sambostock Jul 12, 2022 •

edited

Loading