Introduce ObjectStoreProvider to create an object store based on the url #2906

yahoNanJing · 2022-07-14T08:05:26Z

Which issue does this PR close?

Closes #2905.

Rationale for this change

Currently ObjectStore cannot be self-registered by its path url, which is very important feature for Ballista to support remote storage, like HDFS, etc. To avoid to introduce the remote object store dependency for DataFusion core, it's better to make the ObjectStoreRegistry have the ability to self detect an ObjectStore based on url.

What changes are included in this PR?

Introduce a trait property, ObjectStoreSelfDetector, for the ObjectStoreRegistry for the ability to self detect an ObjectStore based on url. If Ballista wants to have this ability, it needs to set a specific detector for the ObjectStoreRegistry and config it in the RumtimeEnv.

Are there any user-facing changes?

yahoNanJing · 2022-07-14T10:34:44Z

Hi @alamb, @thinkharderdev, @andygrove, could help review this PR? Thanks in advance.

alamb · 2022-07-14T11:13:49Z

cc @tustvold

alamb

Thanks @yahoNanJing I started to look at this PR and the code seems to do what is described in the description 👍 but I was not quite sure I understand the usecase.

Is the primary usecase "lazy" object store registration? Namely that the relevant ObjectStore instance isn't instantiated unless a URL in a plan requires it?

To avoid to introduce the remote object store dependency for DataFusion core,

I agree this is not good.

I thought the way Ballista (or other systems) would work would be that:

The user would provide a configuration file for whatever object stores (e.g. HDFS credentials, or similar) they wanted to connect to
On process startup, Ballista would create ObjectStore instances based on the configuration and register them with the global ObjectStoreRegistry
Since all Ballista processes would have the same (or compatible) configuration, serialized urls created by one would have the correct ObjectStore already instantiated int hem

alamb · 2022-07-14T11:14:37Z

Cargo.toml

@@ -19,7 +19,6 @@
 members = [
    "datafusion/common",
    "datafusion/core",
-    "datafusion/data-access",


this code is unused, correct? And this is a cleanup that is not strictly necessary for this PR? (this is fine, I am just trying to make sure I understand)

Yes. It's for clean up and this PR is based on another PR #2904

yahoNanJing · 2022-07-14T12:08:29Z

Thanks @alamb for your comments.

Is the primary usecase "lazy" object store registration? Namely that the relevant ObjectStore instance isn't instantiated unless a URL in a plan requires it?

Yes. By self-registration, we can achieve the "lazy" registration.

I thought the way Ballista (or other systems) would work would be that:

For the ballista usage, it's necessary for us to introduce the object store extensions as optional features. The detailed usage way may differ for different object stores. For example,

For S3, we need to introduce additional configurations, like bucket, credential info, etc.
For HDFS, related configurations can be found in some specific locations. Therefore, we don't need to do any additional configurations.

alamb · 2022-07-14T13:03:33Z

For the ballista usage, it's necessary for us to introduce the object store extensions as optional features. The detailed usage way may differ for different object stores. For example,

I guess I don't understand why the ObjectStore instances aren't always created when ballista starts up. What value does waiting for a URL to actually refer to them to register them gain?

tustvold

If I understand correctly the issue is following switching to object_store, the stores have to be registered per-host whereas previously they were registered per-scheme. The desired functionality is to be able to register a generic connector that knows given any hdfs URL how to create an appropriate ObjectStore for it.

As such I wonder if we might instead call this concept ObjectStoreSchemeProvider and store a HashMap on ObjectStoreRegistry? What do people think? This would also allow creating an S3SchemeProvider that knows how to connect to any S3 bucket, etc...

tustvold · 2022-07-14T12:53:11Z

datafusion/core/src/datasource/listing/helpers.rs

@@ -162,7 +162,7 @@ pub fn split_files(
 pub async fn pruned_partition_list<'a>(
    store: &'a dyn ObjectStore,
    table_path: &'a ListingTableUrl,
-    filters: &[Expr],
+    filters: &'a [Expr],


Why this change?

When compiling, if without this, it throws lifetime exception.

I think it is a rust complier bug, the original code should not compile.

I also see this error occasionally (but not always) locally

It seems it's the Rust compiler bug. From my understanding for the lifetime, it's necessary for us to add 'a

tustvold · 2022-07-14T12:54:49Z

datafusion/core/src/datasource/object_store.rs

@@ -81,10 +81,19 @@ impl std::fmt::Display for ObjectStoreUrl {
    }
 }

+/// Object store self detector can detector an object store based on the url
+pub trait ObjectStoreSelfDetector: Send + Sync + 'static {


I wonder if we might call this ObjectStoreSchemeProvider, to make it more pluggable??

Actually the functionality for this trait is for getting object store from url.

I think @tustvold 's suggestion is to use a slightly different name that perhaps is more conventional (I think often the term "Provider" is used to describe factory-like things in object oriented programming that instantiate instances of interfaces -- aka what this is doing)

Actually it's based on the url rather than the scheme. How about just rename it to be ObjectStoreProvider

tustvold · 2022-07-14T12:55:19Z

datafusion/core/src/datasource/object_store.rs

 pub struct ObjectStoreRegistry {
    /// A map from scheme to object store that serve list / read operations for the store
-    object_stores: RwLock<HashMap<String, Arc<dyn ObjectStore>>>,
+    object_stores: Arc<RwLock<HashMap<String, Arc<dyn ObjectStore>>>>,
+    self_detector: Option<Arc<dyn ObjectStoreSelfDetector>>,


See above comment, I wonder if instead we make this a HashMap<String, Arc<dyn ObjectStoreSchemeProvider>> keyed by url scheme

I think string is OK

tustvold · 2022-07-14T12:56:35Z

datafusion/core/src/datasource/object_store.rs

+        // First check whether can get object store from registry
+        let store = {
+            let stores = self.object_stores.read();
+            let s = &url[url::Position::BeforeScheme..url::Position::BeforeHost];


I'm not sure about this, the host should not be and is not part of the path passed to ObjectStore implementations

What if we called the detector first and if that does not return an instance, we can fall back to the self.object_stores? This would preserve the existing behavior and would likely have a simpler implementation

Something like (untested):

let store = self_detector // try to get url from detector first .map(|detector| detector.get_by_url(url)) // fallback to looking up by registered scheme .or_else(|| self.oject_stores.read().get(s)) .ok_or_else(|| { DataFusionError::Internal(format!( "No suitable object store found for {}", url )) })?;

I think it's better to call the detector later so that we can reuse the registered object stores.

What I don't like about the current code is that it is hard to concisely explain how urls are resolved as various parts of the URL are tried in a sequence that I find confusing.

I guess I was thinking that we could keep the logic in DataFusion simple, Ballista or some other system could implement whatever arbitrary logic was needed in its implementation of the detector. If the detector wanted to first try the registered object stores it could do so.

Actually the reason why it becomes complex is because the object_store changes the behavior and it's no longer based on the scheme any more. And we have to deal with complex parsing for different object stores.

The self-detector feature based on its url is a necessary feature for the ballista. However, manually registration is also a necessary feature for the datafusion, right? We still need to deal with different kinds of object stores in the datafusion.

If use before path, then it may include credential infos to the key for S3, right? 😭 It's also not good, right?

Why is that not good? I'd expect to be able to register two object stores with the same host but different credentials? I don't follow why this is an issue?

It will easily cause security issues. In general, we should not store credentials in memory, especially directly depending on it.

Could you perhaps expand on the threat model here, and how this would impact it. If the user has opted to use HTTP Basic Auth, I don't see how storing that data in memory as part of the URL is avoidable...

In general, we should not store the users' credential info in memory. The security topic related to many points:

The expiration mechanism

The encoding algorithm

The encryption algorithm

...

However, this PR is not for dealing with it. If anyone is interested in it, we should open another issue. For HDFS, we leverage Kerberos technique to deal with security issue. However, I have little knowledge for S3. My intuition tells me that it's not safe to put the credential info in each request, although it's base64 encoded.

For this PR, it's OK to just use url::Position::BeforeScheme..url::Position::BeforePath to extract the key for one specific object store. And I'll submit a related commit to fix it.

alamb · 2022-07-14T17:57:44Z

If I understand correctly the issue is following switching to object_store, the stores have to be registered per-host whereas previously they were registered per-scheme. The desired functionality is to be able to register a generic connector that knows given any hdfs URL how to create an appropriate ObjectStore for it.

Is this correct @yahoNanJing ? If so then it makes sense to me ( I didn't realize it was per host rather than per-scheme) Does that means it requires an ObjectStore instance per each HDFS URL (hdfs://10.10.0.1 and hdfs://10.10.0.2 would be different ObjectStore instances)?

tustvold · 2022-07-14T18:05:36Z

per each HDFS URL (hdfs://10.10.0.1 and hdfs://10.10.0.2 would be different ObjectStore instances)?

Correct, we only pass the URL path to ObjectStore, and not any authority (host, port, credentials, etc...).

yahoNanJing · 2022-07-15T01:13:58Z

If I understand correctly the issue is following switching to object_store, the stores have to be registered per-host whereas previously they were registered per-scheme. The desired functionality is to be able to register a generic connector that knows given any hdfs URL how to create an appropriate ObjectStore for it.

Hi @alamb and @tustvold, as @tustvold mentioned above, the switching to the object_store changes the previous behavior. And now we need to create ObjectStore for each host.

alamb · 2022-07-15T11:32:42Z

As such I wonder if we might instead call this concept ObjectStoreSchemeProvider and store a HashMap on ObjectStoreRegistry? What do people think? This would also allow creating an S3SchemeProvider that knows how to connect to any S3 bucket, etc...

Sounds like a good idea to me -- depending on @yahoNanJing 's preference, we could also add the feature to DataFusion and then consolidate when it was added / integrated into object_store

alamb

Thanks @yahoNanJing -- I understand the need for this PR and would like to get it in to unblock you if needed

The only thing that this PR needs, in my opinion, is a test (of the ObjectStoreDetector logic)

@tustvold and I had some suggestions on how to improve the code but I think they could be done as a follow on PR as well

alamb · 2022-07-15T11:23:13Z

datafusion/core/src/datasource/listing/helpers.rs

@@ -162,7 +162,7 @@ pub fn split_files(
 pub async fn pruned_partition_list<'a>(
    store: &'a dyn ObjectStore,
    table_path: &'a ListingTableUrl,
-    filters: &[Expr],
+    filters: &'a [Expr],


I also see this error occasionally (but not always) locally

alamb · 2022-07-15T11:24:51Z

datafusion/core/src/datasource/object_store.rs

@@ -81,10 +81,19 @@ impl std::fmt::Display for ObjectStoreUrl {
    }
 }

+/// Object store self detector can detector an object store based on the url
+pub trait ObjectStoreSelfDetector: Send + Sync + 'static {


I think @tustvold 's suggestion is to use a slightly different name that perhaps is more conventional (I think often the term "Provider" is used to describe factory-like things in object oriented programming that instantiate instances of interfaces -- aka what this is doing)

alamb · 2022-07-15T11:29:21Z

datafusion/core/src/datasource/object_store.rs

+        // First check whether can get object store from registry
+        let store = {
+            let stores = self.object_stores.read();
+            let s = &url[url::Position::BeforeScheme..url::Position::BeforeHost];


What if we called the detector first and if that does not return an instance, we can fall back to the self.object_stores? This would preserve the existing behavior and would likely have a simpler implementation

Something like (untested):

let store = self_detector // try to get url from detector first .map(|detector| detector.get_by_url(url)) // fallback to looking up by registered scheme .or_else(|| self.oject_stores.read().get(s)) .ok_or_else(|| { DataFusionError::Internal(format!( "No suitable object store found for {}", url )) })?;

alamb · 2022-07-15T11:35:38Z

datafusion/core/src/datasource/object_store.rs

@@ -81,10 +81,19 @@ impl std::fmt::Display for ObjectStoreUrl {
    }
 }

+/// Object store self detector can detector an object store based on the url


Suggested change

/// Object store self detector can detector an object store based on the url

/// Object store self detector can detector an object store based on the url

///

/// This supports the usecase of providing [`ObjectStore`] instances based on different hosts

// (such as `hdfs://10.10.0.1` and `hdfs://10.10.0.2` which might have different access access credentials)?

Correct, we only pass the URL path to ObjectStore, and not any authority (host, port, credentials, etc...).

yahoNanJing · 2022-07-15T13:39:54Z

Thanks @yahoNanJing -- I understand the need for this PR and would like to get it in to unblock you if needed

The only thing that this PR needs, in my opinion, is a test (of the ObjectStoreDetector logic)

@tustvold and I had some suggestions on how to improve the code but I think they could be done as a follow on PR as well

Actually, this feature is only ballista's interests currently. And I'll add a test in the ballista repo. However, due to the dependency issue among datafusion, ballista and datafusion-objectstore-hdfs. It's necessary for us to merge this PR first. 😢

yahoNanJing · 2022-07-15T13:49:19Z

For someone may be interested, an initial implementation with the integration of datafusion, ballista and objectstore-hdfs is in https://github.com/yahoNanJing/arrow-ballista/tree/dev-202207. And all of the tests passed.

…on the url

codecov-commenter · 2022-07-17T02:30:17Z

Codecov Report

Merging #2906 (2b0c6ad) into master (c528986) will decrease coverage by 0.01%.
The diff coverage is 63.41%.

@@            Coverage Diff             @@
##           master    #2906      +/-   ##
==========================================
- Coverage   85.32%   85.31%   -0.02%     
==========================================
  Files         273      273              
  Lines       49343    49377      +34     
==========================================
+ Hits        42102    42126      +24     
- Misses       7241     7251      +10

Impacted Files	Coverage Δ
datafusion/core/src/datasource/listing/helpers.rs	`94.96% <ø> (ø)`
datafusion/core/src/execution/runtime_env.rs	`75.00% <40.00%> (ø)`
datafusion/core/src/datasource/object_store.rs	`84.44% <64.00%> (-7.23%)`	⬇️
datafusion/core/src/catalog/schema.rs	`82.53% <66.66%> (-1.95%)`	⬇️
datafusion/core/src/execution/context.rs	`78.95% <66.66%> (+0.09%)`	⬆️
datafusion/core/src/physical_plan/planner.rs	`81.04% <100.00%> (ø)`
datafusion/core/tests/user_defined_plan.rs	`87.79% <100.00%> (ø)`
datafusion/expr/src/window_frame.rs	`92.43% <0.00%> (-0.85%)`	⬇️
datafusion/proto/src/bytes/mod.rs	`82.75% <0.00%> (ø)`
datafusion/proto/src/lib.rs	`93.32% <0.00%> (+0.10%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

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

tustvold · 2022-07-17T17:20:19Z

datafusion/core/src/datasource/object_store.rs

+pub trait ObjectStoreProvider: Send + Sync + 'static {
+    /// Detector a suitable object store based on its url if possible
+    /// Return the key and object store
+    fn get_by_url(&self, url: &Url) -> Option<(String, Arc<dyn ObjectStore>)>;


I'm not sure how this returning of a String is intended to work, as it will always be looked up in the hashmap based on &url[url::Position::BeforeScheme..url::Position::BeforePath];

You are right. With the new logic of get_by_url, we can just use the key as the parameter and the returned value without the String key.

tustvold · 2022-07-17T17:21:06Z

datafusion/core/src/datasource/object_store.rs

    /// Create the registry that object stores can registered into.
    /// ['LocalFileSystem'] store is registered in by default to support read local files natively.
-    pub fn new() -> Self {
+    pub fn new_with_detector(


Suggested change

pub fn new_with_detector(

pub fn new_with_provider(

tustvold · 2022-07-17T17:21:12Z

datafusion/core/src/datasource/object_store.rs

 pub struct ObjectStoreRegistry {
    /// A map from scheme to object store that serve list / read operations for the store
-    object_stores: RwLock<HashMap<String, Arc<dyn ObjectStore>>>,
+    object_stores: Arc<RwLock<HashMap<String, Arc<dyn ObjectStore>>>>,
+    self_detector: Option<Arc<dyn ObjectStoreProvider>>,


Suggested change

self_detector: Option<Arc<dyn ObjectStoreProvider>>,

provider: Option<Arc<dyn ObjectStoreProvider>>,

tustvold · 2022-07-17T17:23:35Z

datafusion/core/src/datasource/object_store.rs

+        };
+
+        // If not, then try to detector based on its url.
+        let store = store


This currently has a thread race, it needs to:

Acquire the write lock

Verify that a value hasn't been inserted in the intervening time

Call the detector

Add the result

Drop the write lock

To be honest, this could probably switch to just using a Mutex to keep things simple. There is unlikely to be significant contention to warrant the RWLock

I don't think so. The RWLock is better than just using Mutex. Here, the read case will happen frequently. While the write case only happens a few times.

In the uncontended case, which is extremely likely given how short the critical section is, they will perform exactly the same - if anything the Mutex might be marginally faster. It was more an observation that the complexity of using a RWLock is probably not actually yielding any return.

A simple Mutex would allow you to use get_or_insert_with, and avoid what is currently a thread race

tustvold · 2022-07-17T17:27:37Z

datafusion/core/src/datasource/object_store.rs

 /// Object store registry
+#[derive(Clone)]


Why is this needed, afaict all locations use Arc<ObjectStoreRegistry>?

tustvold · 2022-07-17T17:28:48Z

datafusion/core/src/execution/runtime_env.rs

@@ -121,6 +122,8 @@ pub struct RuntimeConfig {
    pub disk_manager: DiskManagerConfig,
    /// MemoryManager to limit access to memory
    pub memory_manager: MemoryManagerConfig,
+    /// ObjectStoreRegistry to get object store based on url
+    pub object_store_registry: ObjectStoreRegistry,


This should possibly be Arc<ObjectStoreRegistry> for consistency

Why? Other properties are not wrapped with Arc. And for one env, there should be only one runtime_env and its related properties.

Two reasons:

We use Arc<ObjectStoreRegistry> elsewhere and so using it here is more consistent

It provides a hint that this is shared state, e.g. we use Arc<DiskManager> instead of just DiskManager, Arc<MemoryManager> instead of MemoryManager etc...

I see. Agree with you that here better to use Arc

tustvold · 2022-07-18T12:45:44Z

datafusion/core/src/catalog/schema.rs

@@ -162,7 +162,17 @@ mod tests {
    #[tokio::test]
    async fn test_schema_register_listing_table() {
        let testdata = crate::test_util::parquet_test_data();
-        let filename = format!("file:///{}/{}", testdata, "alltypes_plain.parquet");
+        let testdir = if testdata.starts_with('/') {


I wonder if this logic would be better handled in get_data_dir (to remove any trailing /)? As it stands I'm surprised just making this change here is sufficient?

For different OS, the detailed parsing logic is different. It seems the logic here is too trivial and should be refined.

tustvold

I think lets get this in and we can refine it in follow up PRs

ursabot · 2022-07-18T15:01:17Z

Benchmark runs are scheduled for baseline = 6cb695f and contender = 305e265. 305e265 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the core Core DataFusion crate label Jul 14, 2022

yahoNanJing force-pushed the issue-2905 branch from 808014d to d1569f5 Compare July 14, 2022 09:52

yahoNanJing mentioned this pull request Jul 14, 2022

Introduce the object stores in datafusion-contrib as optional features apache/datafusion-ballista#87

Closed

alamb reviewed Jul 14, 2022

View reviewed changes

tustvold reviewed Jul 14, 2022

View reviewed changes

alamb reviewed Jul 15, 2022

View reviewed changes

kyotoYaho added 2 commits July 17, 2022 09:37

Introduce ObjectStoreSelfDetector for detector an object store based …

bdf8209

…on the url

Fix UT

89841a9

yahoNanJing force-pushed the issue-2905 branch from d1569f5 to 89841a9 Compare July 17, 2022 01:56

Fix PR review

7dda307

yahoNanJing force-pushed the issue-2905 branch from bb9044b to 7dda307 Compare July 17, 2022 01:59

tustvold reviewed Jul 17, 2022

View reviewed changes

Fix PR review

2b0c6ad

tustvold reviewed Jul 18, 2022

View reviewed changes

tustvold approved these changes Jul 18, 2022

View reviewed changes

tustvold changed the title ~~Introduce ObjectStoreSelfDetector for detector an object store based on the url~~ Introduce ObjectStoreProvider to create an object store based on the url Jul 18, 2022

tustvold merged commit 305e265 into apache:master Jul 18, 2022

alamb mentioned this pull request Aug 3, 2022

[object store] Parsing of well-known uri formats apache/arrow-rs#2304

Closed

-/// Object store self detector can detector an object store based on the url
+/// Object store self detector can detector an object store based on the url
+///
+/// This supports the usecase of providing [`ObjectStore`] instances based on  different hosts
+//  (such as `hdfs://10.10.0.1` and `hdfs://10.10.0.2` which might have different access access credentials)?
+Correct, we only pass the URL path to ObjectStore, and not any authority (host, port, credentials, etc...).

	self_detector: Option<Arc<dyn ObjectStoreProvider>>,
	provider: Option<Arc<dyn ObjectStoreProvider>>,

Introduce ObjectStoreProvider to create an object store based on the url #2906

Introduce ObjectStoreProvider to create an object store based on the url #2906

Conversation

yahoNanJing commented Jul 14, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

yahoNanJing commented Jul 14, 2022

alamb commented Jul 14, 2022

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yahoNanJing Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

yahoNanJing commented Jul 14, 2022

alamb commented Jul 14, 2022

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

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

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 Jul 14, 2022

tustvold commented Jul 14, 2022

yahoNanJing commented Jul 15, 2022

alamb commented Jul 15, 2022

alamb 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

yahoNanJing commented Jul 15, 2022

yahoNanJing commented Jul 15, 2022 • edited Loading

codecov-commenter commented Jul 17, 2022 • edited Loading

Codecov Report

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 Jul 18, 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

tustvold Jul 18, 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

tustvold left a comment

Choose a reason for hiding this comment

ursabot commented Jul 18, 2022

yahoNanJing Jul 14, 2022 •

edited

Loading

tustvold left a comment •

edited

Loading

yahoNanJing commented Jul 15, 2022 •

edited

Loading

codecov-commenter commented Jul 17, 2022 •

edited

Loading

tustvold Jul 18, 2022 •

edited

Loading

tustvold Jul 18, 2022 •

edited

Loading