Change ObjectStoreRegistry from struct to trait to provide polymorphism #5543

yahoNanJing · 2023-03-10T08:45:59Z

Which issue does this PR close?

Closes #5541.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

…vide polymorphism for get_by_url

yahoNanJing · 2023-03-10T09:41:32Z

Hi @alamb, @tustvold, @thinkharderdev and @andygrove, an initial prototype of the data source cache layer for Ballista has almost been implemented https://github.com/yahoNanJing/arrow-ballista/tree/dev-20230302. From now on, I will continuously reorganize the code and contribute back to the community.

To achieve this, we need to apply a few changes to the datafusion and arrow-rs. Could you help review this PR, first?

tustvold

This seems to have non-trivial overlap with ObjectStoreProvider, I wonder if we could combine them?

On a somewhat related note, I'm a little confused why this is needed for a cache system, couldn't that just be kept as a detail of whatever is creating the ObjectStore? I.e. the stores are registered already wrapped in a CachingStore (or whatever it gets called). I think this would be significantly simpler and easier to understand, and would not require any changes outside of Ballista?

yahoNanJing · 2023-03-10T11:43:37Z

This seems to have non-trivial overlap with ObjectStoreProvider, I wonder if we could combine them?

The ObjectStoreProvider and ObjectStoreManager are for different aspects. As described in the code, ObjectStoreProvider focuses to get ObjectStore when it's not found in cache. While ObjectStoreManager focuses on the polymorphism of how to get an ObjectStore by get_by_url. The main reason to introduce ObjectStoreManager here, is not to break existing usage of ObjectStoreProvider. However, if not considering of existing usage of ObjectStoreProvider, I have no preference on whether to combine them into one.

An example usage is linked on the issue description. https://github.com/yahoNanJing/arrow-ballista/blob/dev-20230302/ballista/core/src/cache_layer/object_store/file.rs

thinkharderdev · 2023-03-10T11:43:49Z

datafusion/execution/src/object_store.rs

+impl ObjectStoreManager for DefaultObjectStoreManager {
+    fn register_store(
+        &self,
+        scheme: &str,
+        host: &str,
+        store: Arc<dyn ObjectStore>,
+    ) -> Option<Arc<dyn ObjectStore>> {
+        let s = format!("{scheme}://{host}");
+        self.object_stores.insert(s, store)
+    }
+
+    /// Get a suitable store for the provided URL. For example:
+    ///
+    /// - URL with scheme `file:///` or no schema will return the default LocalFS store
+    /// - URL with scheme `s3://bucket/` will return the S3 store if it's registered
+    /// - URL with scheme `hdfs://hostname:port/` will return the hdfs store if it's registered
+    fn get_by_url(&self, url: &Url) -> Result<Arc<dyn ObjectStore>> {
+        let s = &url[url::Position::BeforeScheme..url::Position::BeforePath];
+        self.object_stores
+            .get(s)
+            .map(|o| o.value().clone())
+            .ok_or_else(|| {
+                DataFusionError::Internal(format!(
+                    "No suitable object store found for {url}"
+                ))
+            })
+    }
+}


I'm confused by this. This is the same interface as ObjectStoreRegistry. What is the functional difference that requires wrapping this in another layer of indirection?

The reason for this is to make the cache transparent for the caller. An example usage is here,
yahoNanJing/arrow-ballista@48c703b

In Ballista, we can create an ObjectStoreRegistry with a CacheBasedObjectStoreManager. Then every ObjectStore got by ObjectStoreRegistry will be wrapped with a cache layer.

tustvold · 2023-03-10T11:46:23Z

An example usage is linked on the issue description. https://github.com/yahoNanJing/arrow-ballista/blob/dev-20230302/ballista/core/src/cache_layer/object_store/file.rs

Why can't whatever is creating the inner ObjectStore that is passed to https://github.com/yahoNanJing/arrow-ballista/blob/dev-20230302/ballista/core/src/cache_layer/object_store/file.rs#L45 just also create the FileCacheObjectStore and register the FileCacheObjectStore directly? This is the same story as for other ObjectStore decorators that add metrics, tracing, etc...

yahoNanJing · 2023-03-10T11:52:44Z

Why can't whatever is creating the inner ObjectStore that is passed to https://github.com/yahoNanJing/arrow-ballista/blob/dev-20230302/ballista/core/src/cache_layer/object_store/file.rs#L45 just also create the FileCacheObjectStore and register the FileCacheObjectStore directly? This is the same story as for other ObjectStore decorators that add metrics, tracing, etc...

The reason is to make the cache layer transparent for the caller. To achieve this, when invoking get_by_url, we need to get a cache layer wrapped ObjectStore

tustvold · 2023-03-10T12:05:56Z

The reason is to make the cache layer transparent for the caller.

Who is the caller in this case? I had understood Ballista to have a config system that handled object store registration, in order to allow it to propagate to the workers correctly?

when invoking get_by_url, we need to get a cache layer wrapped ObjectStore

This is already covered by ObjectStoreProvider, no? You can just have an ObjectStoreProvider that returns decorated ObjectStore?

yahoNanJing · 2023-03-10T12:44:04Z

This is already covered by ObjectStoreProvider, no? You can just have an ObjectStoreProvider that returns decorated ObjectStore?

When you have already cached the ObjectStore or register the ObjectStore manually, the ObjectStoreProvider will not be effective. Then there's no transparency.

tustvold · 2023-03-10T14:05:10Z

When you have already cached the ObjectStore or register the ObjectStore manually, the ObjectStoreProvider will not be effective. Then there's no transparency.

Is that not an advantage, for example, if registering a InMemory or LocalFileSystem, I would not want them to be automatically wrapped in a caching decorator?

Perhaps am I missing something here, but I had understood the manual registration API being for advanced use-cases where you want fine-grained control of the ObjectStore, with ObjectStoreProvider filling the role of the auto-configuring store, and by extension the only thing transparently supported out-of-the-box. Does Ballista have some mechanism to manually register stores from config / SQL or something?

alamb · 2023-03-11T01:11:18Z

datafusion/execution/src/object_store.rs

+    /// - URL with scheme `file:///` or no schema will return the default LocalFS store
+    /// - URL with scheme `s3://bucket/` will return the S3 store
+    /// - URL with scheme `hdfs://hostname:port/` will return the hdfs store
+    fn get_by_url(&self, url: &Url) -> Result<Arc<dyn ObjectStore>>;


I am sorry I am missing something -- how is this different than

https://github.com/apache/arrow-datafusion/blob/860918d17b6bde396b04d718ee1c76d93054bf11/datafusion/execution/src/object_store.rs#L89

?

While ObjectStoreManager focuses on the polymorphism of how to get an ObjectStore by get_by_url.

It seems like the only function of ObjectStoreProvider is a polymorphic get_by_url 🤔

Thanks @alamb for reviewing. Actually, the main concern is that ObjectStoreProvider only covers the get_by_url. It does not cover the manual registration. Therefore, in this PR, I introduced ObjectStoreManager to cover both. In regard to whether to combine ObjectStoreManager and ObjectStoreProvider, I have no preference. Here, the main reason of not combining them is for the backward compatibility of existing usage of ObjectStoreProvider.

Got it -- thank you for the explanation @yahoNanJing , I missed the register_store

I have no preference. Here, the main reason of not combining them is for the backward compatibility of existing usage of ObjectStoreProvider.

I think we can add a new method to ObjectStoreProvider that has a default implementation and maintain backwards compatibility.

For example, what if we added something like this (not tested) which would require no changes to existing ObjectStoreProviders?

/// Registers the specified object store for urls with scheme/host /// returning the previously registered store if any. fn register_store( &self, scheme: &str, host: &str, store: Arc<dyn ObjectStore>, ) -> Result<Option<Arc<dyn ObjectStore>>> { Err(DataFusionError::NotImplemented("register_store is not supported by this provider")) }

If we could extend ObjectStoreProvider that would be my preference as I think it keeps the overall code simpler

register_store already exists on ObjectStoreRegistry so this would essentially make ObjectStoreRegistry and ObjectStoreProvider have the same interface which I think is confusing. So as I understand it, the role of ObjectStoreProvider is just to allow lazy construction. Manual registration can already be done through the registry. So it just seems to me like we could serve the same use case by holding an Arc<dyn ObjectStoreRegistry> in whatever is managing the cache layer.

I tried to improve the docs in #5577

So it just seems to me like we could serve the same use case by holding an Arc in whatever is managing the cache layer.

Thanks @thinkharderdev. Since now the ObjectStoreRegistry is a struct, I introduced another trait ObjectStoreManager for this with less code change. However, We can just simplify this by changing ObjectStoreRegistry to a trait and provide default implementation with a few code changes.

yahoNanJing · 2023-03-13T01:55:16Z

Is that not an advantage, for example, if registering a InMemory or LocalFileSystem, I would not want them to be automatically wrapped in a caching decorator?

I don't think it's an advantage. How to register an ObjectStore should also be polymorphic. For example, by introducing the ObjectStoreManager, we will be able to use InMemory as the cache layer for LocalFileSystem instead of using LocalFileSystem directly. Otherwise, we won't able to cache the local files.

If you don't want to automatically wrapped in a caching decorator, it's doable to create a specific ObjectStoreManager.

yahoNanJing · 2023-03-13T01:59:54Z

Does Ballista have some mechanism to manually register stores from config / SQL or something?

It depends. For Ballista client, it can manually register stores. And we should not have the assumption that users don't do the registration before using the auto-configured stores registered by ObjectStoreProvider.

yahoNanJing · 2023-03-14T15:02:49Z

Hi @thinkharderdev, @alamb, according to previous discussion, I remove the trait ObjectStoreProvider. Instead, I change the ObjectStoreRegistry from struct to trait, which will be much more simple and straight forward.

alamb

Thanks @yahoNanJing -- I think the structure of this PR now looks very nice 👌 I like how the traits have been combined. It would be good I think to put back the docstrings with examples prior to merge.

I think others should have a chance to weigh in prior to merging, but I really like the result of community discussion

datafusion/execution/src/object_store.rs

thinkharderdev

Nice! I always thought this was a bit convoluted. I think this makes the API more straightforward.

tustvold

Like the switch to a trait object, more flexible and significantly easier to follow 👍

Only question concerns the overlap of register_store and put_with_url. The original design intentionally did not allow registering arbitrary URLs to make it clearer what bits of a URL identified an object store.

I don't feel especially strongly about this, but feel we probably shouldn't have overlapping methods, and definitely should document that things like the path should not influence the returned ObjectStore (as this would have very funky semantics)

tustvold · 2023-03-15T18:15:34Z

datafusion-cli/src/object_storage.rs

+            let store =
+                ObjectStoreScheme::from_str(url.scheme()).map(
+                    |scheme| match scheme {
+                        ObjectStoreScheme::S3 => build_s3_object_store(url),
+                        ObjectStoreScheme::GCS => build_gcs_object_store(url),
+                    },
+                )??;


Suggested change

let store =

ObjectStoreScheme::from_str(url.scheme()).map(

|scheme| match scheme {

ObjectStoreScheme::S3 => build_s3_object_store(url),

ObjectStoreScheme::GCS => build_gcs_object_store(url),

},

)??;

let store = match ObjectStoreScheme::from_str(url.scheme())?

ObjectStoreScheme::S3 => build_s3_object_store(url)?,

ObjectStoreScheme::GCS => build_gcs_object_store(url)?,

};

Or something similar

tustvold · 2023-03-15T18:19:37Z

datafusion/execution/src/object_store.rs

+    /// Insert a [`ObjectStore`] with the key of a given url got by [`get_url_key()`]
+    ///
+    /// If a store with the same url key, it is replaced and returned
+    fn put_with_url(


Do we need both a put_with_url and register_store?

The motivation for not passing the entire URL, was to avoid confusion over what bits of the URL "counted", e.g. should credential influence this? I don't feel strongly, but feel we probably shouldn't have two methods that do basically the same thing

Thanks @tustvold for your suggestion. put_with_url and register_store are using different strategies for registration. Perhaps it's better to just use key and value as the parameters for the interface. I'll refine this in the next commit.

alamb · 2023-03-17T10:44:37Z

Thanks @tustvold for your suggestion. put_with_url and register_store are using different strategies for registration. Perhaps it's better to just use key and value as the parameters for the interface. I'll refine this in the next commit.

@yahoNanJing do you plan to merge this PR and refine in a follow on one, or shall we hold off merging this PR until you have made more changes?

alamb · 2023-03-20T13:30:23Z

What is the plan for this PR?

yahoNanJing · 2023-03-21T01:30:16Z

Hi @alamb, sorry for the delay. I'm going to add commit to this PR for the review comments today

yahoNanJing · 2023-03-21T03:21:52Z

Hi @alamb, @thinkharderdev, @tustvold, just add a commit for your comments. Could you help have a review again?

tustvold · 2023-03-21T06:15:52Z

datafusion-cli/src/object_storage.rs

+impl ObjectStoreRegistry for DatafusionCliObjectStoreRegistry {
+    fn register_store(
+        &self,
+        key: &str,


What is the use-case for registering stores not based on scheme and host? I mainly ask as it is unclear how this method interacts with get_by_url? Could we just keep the host and scheme signature instead?

Do we need to consider port? Sometimes, it's also important to identify a url. I don't know whether it's too strict to register with host and scheme. For previous implementation, for a given url, the key for registration is &url[url::Position::BeforeScheme..url::Position::BeforePath] which may be not the same as your expectation, although I agree with you it's should not include the credential info

the key for registration is

That's the key for lookup, and so it was harmless for it to contain more, it would just not be found.

port

Including an optional port seems like a sensible addition

If with only scheme and host for register, how can we achieve register by url which may be invoked in get_with_url for lazy registration with key of &url[url::Position::BeforeScheme..url::Position::BeforePath]

That would be a bug then, yes. We shouldn't be registering with that, well spotted.

It should instead call register_store with just the relevant parts of the URL

I'm OK with either with url or a pair of (scheme, host, port).

Let's go with a URL, it is more flexible, avoids a potentially breaking change even if it was unintentional, and also matches the get method

OK. I'll refine it

Thank you, apologies for the back and forth 😄

Np. We are making things better and better. Thanks for your suggestions to let the interface simply and meaningful.

tustvold · 2023-03-21T09:50:02Z

datafusion/execution/src/object_store.rs

-                    let key =
-                        &url[url::Position::BeforeScheme..url::Position::BeforePath];
-                    self.object_stores.insert(key.to_owned(), store.clone());


As pointed out in https://github.com/apache/arrow-datafusion/pull/5543/files#r1143108925 this has a bug, in that it registers using the entire URL authority, including things like credentials, etc... This should instead be calling register_store as "normal".

tustvold

Mostly just some minor nits, looks good to me, thank you

tustvold · 2023-03-21T11:34:59Z

datafusion/execution/src/object_store.rs

+    /// If no [`ObjectStore`] found for the `url`, ad-hoc discovery may be executed depending on
+    /// the `url`. An [`ObjectStore`] may be lazily created and registered.


Suggested change

/// If no [`ObjectStore`] found for the `url`, ad-hoc discovery may be executed depending on

/// the `url`. An [`ObjectStore`] may be lazily created and registered.

/// If no [`ObjectStore`] found for the `url`, ad-hoc discovery may be executed depending on

/// the `url` and [`ObjectStoreRegistry`] implementation

tustvold · 2023-03-21T11:35:39Z

datafusion/execution/src/object_store.rs

@@ -132,18 +115,37 @@ pub trait ObjectStoreProvider: Send + Sync + 'static {
 /// buckets using [`ObjectStoreRegistry::register_store`]
 ///
 /// 2. Systems relying on ad-hoc discovery, without corresponding DDL, can create [`ObjectStore`]
-/// lazily, on-demand using [`ObjectStoreProvider`]
+/// lazily


Suggested change

/// lazily

/// lazily by providing a custom implementation of [`ObjectStoreRegistry`]

tustvold · 2023-03-21T11:40:47Z

datafusion/execution/src/object_store.rs

+    let port_info = url
+        .port()
+        .map(|port| format!(":{port}"))
+        .unwrap_or(String::new());
+    format!(
+        "{}://{}{}/",
+        &url[url::Position::BeforeScheme..url::Position::AfterScheme],
+        &url[url::Position::BeforeHost..url::Position::AfterHost],
+        port_info
+    )


Suggested change

let port_info = url

.port()

.map(|port| format!(":{port}"))

.unwrap_or(String::new());

format!(

"{}://{}{}/",

&url[url::Position::BeforeScheme..url::Position::AfterScheme],

&url[url::Position::BeforeHost..url::Position::AfterHost],

port_info

)

format!(

"{}://{}{}",

url->scheme(),

&url[url::Position::BeforeHost..url::Position::AfterPort],

)

Note: this also removes the trailing / which I think isn't correct

My bad. The trailing / can be removed.

tustvold · 2023-03-21T11:42:41Z

datafusion/execution/src/object_store.rs

+        let url = Url::parse("file://").unwrap();
+        let key = get_url_key(&url);
+        object_stores.insert(key, Arc::new(LocalFileSystem::new()));


Suggested change

let url = Url::parse("file://").unwrap();

let key = get_url_key(&url);

object_stores.insert(key, Arc::new(LocalFileSystem::new()));

object_stores.insert("file://".to_string(), Arc::new(LocalFileSystem::new()));

See below comment about removing trailing /

tustvold · 2023-03-21T11:43:43Z

datafusion/execution/src/object_store.rs

+    /// Get a suitable store for the provided URL. For example:
+    ///
+    /// - URL with scheme `file:///` or no schema will return the default LocalFS store
+    /// - URL with scheme `s3://bucket/` will return the S3 store
+    /// - URL with scheme `hdfs://hostname:port/` will return the hdfs store


Suggested change

/// Get a suitable store for the provided URL. For example:

///

/// - URL with scheme `file:///` or no schema will return the default LocalFS store

/// - URL with scheme `s3://bucket/` will return the S3 store

/// - URL with scheme `hdfs://hostname:port/` will return the hdfs store

/// Get a suitable store for the provided URL

These are outdated now

tustvold · 2023-03-21T11:47:51Z

datafusion/execution/src/object_store.rs

+}
+
+/// The default [`ObjectStoreRegistry`]
+pub struct DefaultObjectStoreRegistry {


Suggested change

pub struct DefaultObjectStoreRegistry {

///

/// Stores are registered based on the scheme, host and port of the provided URL

/// with a [`LocalFileSystem::new`] automatically registered for `file://`

///

/// For example:

///

/// - `file:///my_path` will return the default LocalFS store

/// - `s3://bucket/path` will return a store registered with `s3://bucket` if any

/// - `hdfs://host:port/path` will return a store registered with `hdfs://host:port` if any

pub struct DefaultObjectStoreRegistry {

tustvold · 2023-03-21T11:50:17Z

datafusion-cli/src/object_storage.rs

+        self.inner.register_store(url, store)
+    }
+
+    fn get_or_lazy_register_store(&self, url: &Url) -> Result<Arc<dyn ObjectStore>> {


Suggested change

fn get_or_lazy_register_store(&self, url: &Url) -> Result<Arc<dyn ObjectStore>> {

fn get_store(&self, url: &Url) -> Result<Arc<dyn ObjectStore>> {

The lazy creation is an implementation detail of certain ObjectStoreRegistry, I'm not sure it needs to be encoded in the name

tustvold · 2023-03-21T11:53:47Z

datafusion/execution/src/object_store.rs

+    /// The [`DefaultObjectStoreRegistry`] will only depend on the inner object store cache
+    /// to decide whether it's able to find an [`ObjectStore`] for a url. No ad-hoc discovery
+    /// and lazy registration will be executed.


Suggested change

/// The [`DefaultObjectStoreRegistry`] will only depend on the inner object store cache

/// to decide whether it's able to find an [`ObjectStore`] for a url. No ad-hoc discovery

/// and lazy registration will be executed.

These docs aren't rendered by docs.rs, I moved this onto the type definition and reworded it slightly

tustvold · 2023-03-21T11:54:23Z

datafusion-cli/src/object_storage.rs

@@ -43,28 +46,52 @@ impl FromStr for ObjectStoreScheme {
    }
 }

-#[derive(Debug)]
-pub struct DatafusionCliObjectStoreProvider {}
+#[derive(Debug, Default)]


Suggested change

#[derive(Debug, Default)]

/// An [`ObjectStoreRegistry`] that can automatically create S3 and GCS stores for a given URL

#[derive(Debug, Default)]

alamb · 2023-03-21T19:48:02Z

Thanks for sticking with this @yahoNanJing and @tustvold

Introduce ObjectStoreManager trait for the ObjectStoreRegistry to pro…

5bb99a1

…vide polymorphism for get_by_url

yahoNanJing requested review from alamb and tustvold March 10, 2023 08:46

tustvold reviewed Mar 10, 2023

View reviewed changes

thinkharderdev reviewed Mar 10, 2023

View reviewed changes

alamb reviewed Mar 11, 2023

View reviewed changes

This was referenced Mar 13, 2023

Add as_any() to the ObjectStore to make it able to identify which ObjectStore is using for the related trait object apache/arrow-rs#3839

Closed

Minor: improve docstrings for ObjectStoreRegistry and ObjectStoreProvider #5577

Merged

Change ObjectStoreRegistry from struct to trait

84c1338

github-actions bot added the core Core DataFusion crate label Mar 14, 2023

Merge branch 'main' into issue-5541

69a0202

alamb approved these changes Mar 15, 2023

View reviewed changes

datafusion/execution/src/object_store.rs Show resolved Hide resolved

thinkharderdev approved these changes Mar 15, 2023

View reviewed changes

tustvold reviewed Mar 15, 2023

View reviewed changes

Fix for review comments

eb483a5

tustvold reviewed Mar 21, 2023

View reviewed changes

Minor fix

2b93855

tustvold reviewed Mar 21, 2023

View reviewed changes

kyotoYaho added 3 commits March 21, 2023 18:34

Use url instead of &str as the parameter for register_store

87fce65

Remove register_with_scheme_and_host

33dee54

Minor fix

385ea56

tustvold approved these changes Mar 21, 2023

View reviewed changes

yahoNanJing force-pushed the issue-5541 branch from 21e050d to 9074fb4 Compare March 22, 2023 02:38

Fix for review comments

7bc8603

yahoNanJing force-pushed the issue-5541 branch from 9074fb4 to 7bc8603 Compare March 22, 2023 02:46

yahoNanJing changed the title ~~Introduce ObjectStoreManager trait for the ObjectStoreRegistry to provide polymorphism for get_by_url~~ Change ObjectStoreRegistry from struct to trait to provide polymorphism Mar 22, 2023

yahoNanJing merged commit 3a8b5da into apache:main Mar 22, 2023

		/// If no [`ObjectStore`] found for the `url`, ad-hoc discovery may be executed depending on
		/// the `url`. An [`ObjectStore`] may be lazily created and registered.

	/// lazily
	/// lazily by providing a custom implementation of [`ObjectStoreRegistry`]

-pub struct DefaultObjectStoreRegistry {
+///
+/// Stores are registered based on the scheme, host and port of the provided URL
+/// with a [`LocalFileSystem::new`] automatically registered for `file://`
+///
+/// For example:
+///
+/// - `file:///my_path` will return the default LocalFS store
+/// - `s3://bucket/path` will return a store registered with `s3://bucket` if any
+/// - `hdfs://host:port/path` will return a store registered with `hdfs://host:port` if any
+pub struct DefaultObjectStoreRegistry {

	fn get_or_lazy_register_store(&self, url: &Url) -> Result<Arc<dyn ObjectStore>> {
	fn get_store(&self, url: &Url) -> Result<Arc<dyn ObjectStore>> {

	/// The [`DefaultObjectStoreRegistry`] will only depend on the inner object store cache
	/// to decide whether it's able to find an [`ObjectStore`] for a url. No ad-hoc discovery
	/// and lazy registration will be executed.

	#[derive(Debug, Default)]
	/// An [`ObjectStoreRegistry`] that can automatically create S3 and GCS stores for a given URL
	#[derive(Debug, Default)]

Change ObjectStoreRegistry from struct to trait to provide polymorphism #5543

Change ObjectStoreRegistry from struct to trait to provide polymorphism #5543

Conversation

yahoNanJing commented Mar 10, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

yahoNanJing commented Mar 10, 2023

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

yahoNanJing commented Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Mar 10, 2023 • edited Loading

yahoNanJing commented Mar 10, 2023

tustvold commented Mar 10, 2023

yahoNanJing commented Mar 10, 2023

tustvold commented Mar 10, 2023

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

yahoNanJing Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

yahoNanJing commented Mar 13, 2023 • edited Loading

yahoNanJing commented Mar 13, 2023

yahoNanJing commented Mar 14, 2023

alamb left a comment

Choose a reason for hiding this comment

thinkharderdev left a comment

Choose a reason for hiding this comment

tustvold left a comment • 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

alamb commented Mar 17, 2023

alamb commented Mar 20, 2023

yahoNanJing commented Mar 21, 2023

yahoNanJing commented Mar 21, 2023

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

tustvold 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

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

alamb commented Mar 21, 2023

tustvold left a comment •

edited

Loading

yahoNanJing commented Mar 10, 2023 •

edited

Loading

tustvold commented Mar 10, 2023 •

edited

Loading

yahoNanJing Mar 14, 2023 •

edited

Loading

yahoNanJing commented Mar 13, 2023 •

edited

Loading

tustvold left a comment •

edited

Loading