BULK-22, #398: Support reading s3 urls #399

DavidTaylorCengage · 2022-02-08T16:39:14Z

Purpose

Add the ability to load records into Cassandra directly from AWS S3, instead of needing to write awkward scripts that load files one-by-one.

Fixes #398

Implementation Notes

Since this is "new functionality added in a backwards compatible manner," I have given the version a minor bump to 1.9.0-SNAPSHOT.

The only way I could find to get the S3 config where I needed it was to set the Config object into BulkLoaderURLStreamHandlerFactory from DataStaxBulkLoader.run(), then pass it into the handlers via provider.maybeCreateURLStreamHandler(). If someone knows a better/cleaner way to achieve this, let me know. :)

Since the construction of the URLStreamHandlerProvider implementations is done by ServiceLoader, I was not able to get the building of the S3Client in S3URLStreamHandlerProvider's constructor. Instead, it calls an init() method when maybeCreateURLStreamHandler() is passed an s3:// URL, with a flag to prevent re-initialization. As before, if someone knows a nicer way to achieve this, I'm happy to make adjustments.

Lastly, I added a few things to CONTRIBUTING.md that I discovered while implementing this feature that I could not find documented anywhere else. If there are any other improvements to documentation that folks want to sneak in...

Testing

To test this new feature, you're going to need an S3 bucket with some records in it. I used JSON, but CSV should work as well. You can use either a profile or the access/secret key method for authorization.

Choose some URLs from your S3 bucket. Both individual URLs and a urlfile will work.
Run dsbulk load <options>. I used a config file to provide my options, but you can pass them as arguments as well. The key is to include the dsbulk.s3.* options, per the updated documentation.
Observe the records loaded into the appropriate Cassandra tables.

Notes

This is my first contribution to this repository, so if I've done anything wrong, used the incorrect idioms, or gone against policy, please let me know and I'm happy to make corrections. :) I have signed the CLA.

Motivation: In order to efficiently upload a large number of records from AWS S3 to Cassandra, dsbulk needs to be able to read from s3:// URLs. Modifications: The main change here is adding the S3URLStreamHandler(Provider) classes. This makes reading from S3 URLs transparent to the rest of the application in the same way that reading from stdin/stdout is. A modifcation to DataStaxBulkLoader was required to be able to pass the config into these new classes. Result: Users may now pass s3:// URLs to load commands and dsbulk will retrieve the content from AWS S3 and load it into Cassandra. Future Work: For this use case, only reading from S3 was required, but adding support for writing to S3 should be a fairly simple modification to S3URLStreamHandler.getOutputStream().

I also added some new documentation to CONTRIBUTING.md for other new contributors who might not know some of the details of the local build process.

DavidTaylorCengage · 2022-02-10T16:11:14Z

Does anyone know why four checks are still "expected", and only the one has completed?

adutra · 2022-02-11T14:33:56Z

Does anyone know why four checks are still "expected", and only the one has completed?

Some webhooks seem broken, and some of the checks are not accessible outside of DataStax unfortunately.

And btw thank you for this PR, in overall it seems solid. I will try to get some time to review it properly next week.

DavidTaylorCengage · 2022-02-11T15:35:31Z

Sounds good. Thanks! :)

adutra · 2022-02-18T21:15:52Z

@DavidTaylorCengage I'm late on my schedule but I am about to start reviewing your PR.

The only way I could find to get the S3 config where I needed it was to set the Config object into BulkLoaderURLStreamHandlerFactory from DataStaxBulkLoader.run(), then pass it into the handlers via provider.maybeCreateURLStreamHandler(). If someone knows a better/cleaner way to achieve this, let me know. :)
Since the construction of the URLStreamHandlerProvider implementations is done by ServiceLoader, I was not able to get the building of the S3Client in S3URLStreamHandlerProvider's constructor. Instead, it calls an init() method when maybeCreateURLStreamHandler() is passed an s3:// URL, with a flag to prevent re-initialization. As before, if someone knows a nicer way to achieve this, I'm happy to make adjustments.

First of all, thank you so much for explaining your design choices. I do understand the issue, but indeed it seems a bit overkill to pass the whole DSBulk config in order to create a URL handler.

Let's take a step back.

When I saw this PR for the first time, I immediately thought about JDBC URLs and how they can carry a lot of information. I was expecting something similar here. For example, a MySQL URL is something like mysql://<username>:<password>@<host>:<port>/<db_name>.

For S3, according to this doc, the equivalent would be a so-called S3 virtual-hosted-style URL, e.g.: https://bucket-name.s3.Region.amazonaws.com/key-name.

The configuration options we need, according to this PR, are:

region: always required
credentials: one of the below:
- accessKeyId + secretAccessKey; or
- profile

It seems that the region could easily be included in the URL. Did you consider this option?

BTW one thing that is missing IMO is an example of an S3 URL: there is none in this PR. How do they look like? Do they include the region and the bucket name? It seems that you consider the entire host as the bucket name, so it seems a URL would be like s3://my-bucket/key-name. Why not use virtual-hosted URLs instead?

For the credentials, it would indeed be bad to include them in the URL, especially secretAccessKey (even though MySQL for example allows passwords in URLs). But putting those credentials in DSBulk is also a bit insecure, and VERY insecure if the user passes the credentials via the command line.

It seems from your PR that the credentials are used to create an AwsCredentialsProvider. It also seems that if none is provided, the AWS client is able to read the default AWS configuration files, typically ~/.aws/credentials according to this doc.

So my main question now is: shouldn't we rely exclusively on ~/.aws/credentials? This IMO would eliminate the need to pass the config to the URL handler factory:

The region would be specified in the URL itself, using virtual-hosted-style URLs; (this also has the advantage of using standard URL formats)
The credentials would be read from ~/.aws/credentials.

Does that make sense to you? I am not an AWS specialist so please forgive me if I am completely off track.

adutra · 2022-02-18T21:26:53Z

A few more notes:

S3 virtual-hosted-style URLs have an https scheme. This obviously wouldn't work here, so we would have to use the s3 scheme as you did. To be complete I was thinking about something close to this syntax:

s3://<bucket>.s3.<region>.amazonaws.com/<key>

Example:

s3://my-bucket.s3.us-west-2.amazonaws.com/export.csv

Including the region in the URL would probably force you to create one AWS client per call to openConnection – as opposed to creating one single, shared instance of S3Client. In fact I think I would prefer that. It wouldn't be a performance problem for DSBulk since it would call openConnection only once at the beginning of the operation.
Finally, if you prefer to leave the region out of the URL, that could work with me too. It seems that S3Client is perfectly able to locate the region in the usual locations.

DavidTaylorCengage · 2022-02-18T22:11:12Z

I also am not an AWS specialist, so I've only ever used the s3://my-bucket/key-name format. I'll have to take a look at the s3://<bucket>.s3.<region>.amazonaws.com/<key> format and see if that would work. How would you feel about a "custom format" like s3://<profile>.<bucket>.<region>.s3/<key>, from which we extract the right credentials and then build a canonical URL? It's a hack, but it might solve the credentials problem if we can't find a better solution.

The reason why we can't 100% rely on ~/.aws/credentials is because you can include multiple profiles in that file; I've actually got five different profiles for the various different buckets I use. Thus, we at least need some way to specify which set of credentials to use.

Certainly I agree that passing the accessKeyId and secretAccessKey is insecure. I included that for folks who "just need to upload some data" and didn't want to go through the whole AWS credentials setup process, or who, for whatever reason (running from a cloud instance, etc.), weren't able to use the ~/.aws/credentials file (although in that case, they should look into using role-based access...). I'm happy to remove the temptation to use insecure options if you would like, though.

It wouldn't be a performance problem for DSBulk since it would call openConnection only once at the beginning of the operation.

I could be mistaken, but it looks like it will call openConnection once per URL? So if we had a -urlfile specified, wouldn't it open a new connection for each line in that file? Our Staging environment had something like 8.8 million records saved in S3, so creating only a single instance of S3Client would be a really good idea!

Thanks for the feedback! These definitely sound like some good potential improvements. I'll start addressing your comments on Monday. :)

adutra · 2022-02-19T19:03:43Z

How would you feel about a "custom format" like s3://<profile>.<bucket>.<region>.s3/<key>, from which we extract the right credentials and then build a canonical URL? It's a hack, but it might solve the credentials problem if we can't find a better solution.

Not sure I follow: how would you extract the credentials from a URL like s3://<profile>.<bucket>.<region>.s3/<key>?

Regarding the proposed format: afaict profile and region are optional, so I wonder if they wouldn't be better expressed as query parameters? E.g. s3://<bucket>/<key>?profile=<profile>&region=<region>. We are diverging from the virtual-hosted-style format, but I could live with that if that's better for you.

The reason why we can't 100% rely on ~/.aws/credentials is because you can include multiple profiles in that file; I've actually got five different profiles for the various different buckets I use. Thus, we at least need some way to specify which set of credentials to use.

OK so this could be addressed by using a URL format that includes the profile, right?

Certainly I agree that passing the accessKeyId and secretAccessKey is insecure. I included that for folks who "just need to upload some data" and didn't want to go through the whole AWS credentials setup process, or who, for whatever reason (running from a cloud instance, etc.), weren't able to use the ~/.aws/credentials file (although in that case, they should look into using role-based access...). I'm happy to remove the temptation to use insecure options if you would like, though.

I guess I would prefer that indeed. I'm pretty sure it won't be long until we start receiving complaints that DSBulk allows AWS secrets to be stored in insecure config files :-) Now if you really really need it, how about putting these in the URL too? E.g. s3://<bucket>/<key>?profile=<profile>&region=<region>&accessKeyId=<accessKeyId>&secretAccessKey=<secretAccessKey>? Then we could log a big fat warning telling the user not to do it.

I could be mistaken, but it looks like it will call openConnection once per URL? So if we had a -urlfile specified, wouldn't it open a new connection for each line in that file? Our Staging environment had something like 8.8 million records saved in S3, so creating only a single instance of S3Client would be a really good idea!

Indeed in the case of a urlfile, we would open N connections, one per line. But urfiles usually contain only a handful of lines. Are you saying that you have urfiles with 8.8 million lines in them? That's huge indeed, we didn't conceived urlfiles to work with so many lines. I'm even surprised that it worked for you!

DavidTaylorCengage · 2022-02-21T23:25:39Z

Not sure I follow: how would you extract the credentials from a URL like s3://<profile>.<bucket>.<region>.s3/<key>?

Probably using String.split(".") on the hostname. From that, we can build the s3://<bucket>.s3.<region>.amazonaws.com/<key> or s3://<bucket>/<key> URLs and provide the right credentials to an S3ClientBuilder.

OK so this could be addressed by using a URL format that includes the profile, right?

Correct, that was my thinking. Upon further reflection, though, I think it's unlikely that we'll have a use case where the user needs to provide multiple sets of credentials for a single run of dsbulk, so maybe passing the same values in every URL is excessive?

I guess I would prefer that indeed. I'm pretty sure it won't be long until we start receiving complaints that DSBulk allows AWS secrets to be stored in insecure config files :-) Now if you really really need it, how about putting these in the URL too? E.g. s3:///?profile=&region=&accessKeyId=&secretAccessKey=? Then we could log a big fat warning telling the user not to do it.

I'll delete these config options the next time I push changes. :) Once we work out the right way to pass credentials -- maybe via URL -- then I'll make sure to include a warning for those using the insecure method.

Indeed in the case of a urlfile, we would open N connections, one per line. But urfiles usually contain only a handful of lines. Are you saying that you have urfiles with 8.8 million lines in them? That's huge indeed, we didn't conceived urlfiles to work with so many lines. I'm even surprised that it worked for you!

Hehe. 😅 I briefly mentioned it in the #398 issue description: the system that is writing records to S3 is multi-node and multi-thread, so we can't really combine the records into a smaller number of files (at write-to-s3-time, anyways). With one record per file, we end up with a pretty ridiculous number of files. We did get several out-of-memory errors when running this branch, so I recommended breaking the urlfile into multiple pieces, but haven't heard back from our Cassandra admin if that was successful (weekend and holiday and all).

If we think it's reasonable to impose an explicit or implicit limit on the number of files contained in a urlfile, I can work with that. Maybe we can use a singleton to prevent re-initializing with the same parameters?

adutra · 2022-02-25T14:04:55Z

I'll delete these config options the next time I push changes. :) Once we work out the right way to pass credentials -- maybe via URL -- then I'll make sure to include a warning for those using the insecure method.

I will let you decide which URL format works best. And also decide whether we should include credentials in the URL or leave them out and fetch them in the credentials file. As long as we don't use DSBulk config for that, I think I am Ok – this was indeed the only design choice that was making me uncomfortable.

We did get several out-of-memory errors when running this branch, so I recommended breaking the urlfile into multiple pieces

Yes, that is inevitable since the urfile lines are stored in a java.util.List in memory.

This is a separate issue though. As a workaround I'd indeed suggest that you split your urlfiles. However if you want to, I'd suggest that you open an issue to improve this. It's "just" a matter of turning that List into a (lazy) Stream or better yet, a Flux. I suspect however that the fix won't be trivial and will incur substantial changes to AbstractFileBasedConnector – but nothing impossible.

But that leaves us with this other decision to make: whether to use a single S3 client for all URLs, or a dedicated one for each. We'd probably need to benchmark a bit and figure out what is a reasonable maximum size for a urlfile.

changelog/README.md

url/pom.xml

adutra · 2022-03-08T13:38:15Z

Hi @DavidTaylorCengage just checking in to see if you need anything from me? FYI we would like to release 1.9.0 at the end of the month, it would be great to get this in (could be in preview status if we are unsure about the URL format, etc.). Thanks!

DavidTaylorCengage · 2022-03-08T15:46:40Z

Hello @adutra ! Sorry for the delay. This is on my to-do list, but it's been a busy week. I'll try to finish it off by the end of this week, though. As I understand it, there are three outstanding tasks:

Move the release notes. (This is already done, just not pushed.)
Resolve the credentials issue, probably by using a custom URL format.
Address the "S3 client per URL vs S3 client for all URLs" question. I'll see if I can do some performance testing locally to give us an idea of the relative performance of these options. This decision might also be made for us depending on the credentials solution.

Does that sound right/good to you?

adutra · 2022-03-09T08:35:25Z

Does that sound right/good to you?

Absolutely, thank you!

DavidTaylorCengage · 2022-03-11T23:40:29Z

Alas, I was busier than I expected today, and didn't finish either of the two remaining tasks. 😞 I did have some time to think about it, though, and I want to try adding a cache to S3URLStreamHandler so that when I put the credentials on each URL (as query parameters), we don't spend a lot of time re-creating the same client over and over again. I'm not sure if this cache should be adjustable, or a fixed size. As far as I know now, this will be my highest priority on Monday, so I should be able to get something done! 😉

Specifically, in addition to a few minor clean-up tasks, I switched from specifying the S3 credentials in the config to specifying them as query params on the individual URLs. To alleviate the performance concerns about creating a new client for each URL in a urlfile, I have added a cache with a configurable size. I also added warnings when someone passes S3 credentials in an insecure way or when they use a urlfile with over 100,000 lines (which can cause out-of-memory errors). This limit is pretty arbitrary, and may be adjusted in the future.

ErickRamirezAU

David, first up -- thank you for this very valuable contribution! This is very useful and there is definitely demand for it. 🍻

Just thinking out loud, I was wondering if it would be easy for a user to determine which JSON files were or were not processed:

will the JSON filenames be sent to STDOUT?
will the JSON filenames be persisted to the job log?

ErickRamirezAU · 2022-03-16T06:50:37Z

manual/settings.md

+
+#### --s3.clientCacheSize<br />--dsbulk.s3.clientCacheSize _&lt;number&gt;_
+
+The size (count) of the S3Client cache. Since each S3 URL


I'd like to request that we put in additional guidance here. It's not readily obvious to me why 20 is a good default.

For example, what happens if we set the cache size to 100? or 5? Thoughts? 🙂

I pretty much just picked an arbitrary number. 🤷‍♂️ In our use case, we're using the same credentials for all the URLs, so the cache only ever has one entry. I suspect using the same credentials for all the URLs will be the usual use case, and it would be a bit of a pain (and somewhat unexpected) to supply more than 20 different sets of credentials in a single urlfile. I suppose that is actually an argument for a cache size of 1. 🙃

I'm happy to add a sentence explaining this if you want, though. Do note, however, that this file (settings.md) and application.template.conf are automatically generated from dsbulk-reference.conf, so the same explanatory statement will be shared between the three files.

ErickRamirezAU · 2022-03-16T06:53:54Z

url/src/main/java/com/datastax/oss/dsbulk/url/S3URLStreamHandlerProvider.java

+public class S3URLStreamHandlerProvider implements URLStreamHandlerProvider {
+
+  private static final String S3CLIENT_CACHE_SIZE_PATH = "dsbulk.s3.clientCacheSize";
+  private static final int DEFAULT_S3CLIENT_CACHE_SIZE = 20;


In-line with my request above, could we add a one-line comment on why 20 is a good default please? 🙏

ErickRamirezAU · 2022-03-16T06:58:44Z

workflow/commons/src/main/resources/dsbulk-reference.conf

@@ -954,6 +954,14 @@ dsbulk {

  }

+  # Settings applicable for reading from AWS S3 URLs.
+  s3 {
+    # The size (count) of the S3Client cache. Since each S3 URL


Whatever additional guidance gets included in settings.md, please add them here too. 🙏

ErickRamirezAU · 2022-03-16T07:02:29Z

manual/application.template.conf

+    # Settings applicable for reading from AWS S3 URLs.
+    ################################################################################################
+
+    # The size (count) of the S3Client cache. Since each S3 URL


Whatever additional guidance gets included in settings.md, please add them here too. 🙏

url/src/main/java/com/datastax/oss/dsbulk/url/S3URLStreamHandler.java

DavidTaylorCengage · 2022-03-17T15:45:08Z

Just thinking out loud, I was wondering if it would be easy for a user to determine which JSON files were or were not processed:

will the JSON filenames be sent to STDOUT?

will the JSON filenames be persisted to the job log?

Yes, the file names appear in positions.txt, and in operation.log/STDOUT if you have debulk.log.verbosity = 2.

adutra · 2022-04-01T08:55:31Z

@DavidTaylorCengage just a heads up that we are planning to release 1.9.0 today or beginning of next week. Would it be OK with you if we push this PR to 1.10.0?

DavidTaylorCengage · 2022-04-01T14:51:09Z

Yes, that's fine with me. 🙂

Also update URL-query-parsing logic and document the reason for choosing 20 as the cache size.

url/src/main/java/com/datastax/oss/dsbulk/url/S3URLStreamHandler.java

DavidTaylorCengage · 2022-08-25T17:30:09Z

I think I've addressed all the feedback? Let me know if there are any additional changes you would like me to make. :)

DavidTaylorCengage · 2022-11-17T21:55:26Z

@adutra ? Does this PR satisfy? I'm happy to make any additional changes you want me to. :)

adutra · 2022-11-29T14:45:41Z

@DavidTaylorCengage I'm sorry, I was pulled out of this project but I'll make one last pass today or tomorrow, then some of my colleagues will take it from there.

adutra · 2022-11-29T17:35:42Z

Trying to sum up the conversations we had so far:

URL format: there was some back and forth, but the current format and parsing seem both OK to me.
Client per URL vs cached Client: given that the requirements include being able to consume urlfiles with millions of lines, a cache is probably necessary anyways. So OK from my point of view.
- ~~Note to myself: create an issue to "officialize" the support for urlfiles with up to millions of lines.~~ DONE

DavidTaylorCengage added 3 commits February 7, 2022 14:39

BULK-22, datastax#398: Bump minor version in preparation for new feature

ce3e88f

BULK-22, datastax#398: Add documentation for new read-from-S3 feature

d29a0b4

I also added some new documentation to CONTRIBUTING.md for other new contributors who might not know some of the details of the local build process.

DavidTaylorCengage changed the title ~~Bulk 22, #398: Support reading s3 urls~~ BULK-22, #398: Support reading s3 urls Feb 8, 2022

Merge branch '1.x' into BULK-22-398-support-reading-s3-urls

983c767

BULK-22, datastax#398: Remove insecure S3 credentials config options

9973b16

adutra reviewed Feb 25, 2022

View reviewed changes

changelog/README.md Show resolved Hide resolved

url/pom.xml Show resolved Hide resolved

ErickRamirezAU suggested changes Mar 16, 2022

View reviewed changes

adutra reviewed Mar 16, 2022

View reviewed changes

url/src/main/java/com/datastax/oss/dsbulk/url/S3URLStreamHandler.java Outdated Show resolved Hide resolved

url/src/main/java/com/datastax/oss/dsbulk/url/S3URLStreamHandler.java Outdated Show resolved Hide resolved

DavidTaylorCengage added 2 commits April 8, 2022 13:50

BULK-22, datastax#398: Use Caffeine instead of LRUMap for caching

0e4e27c

Also update URL-query-parsing logic and document the reason for choosing 20 as the cache size.

Merge branch '1.x' into BULK-22-398-support-reading-s3-urls

c53448d

adutra reviewed Apr 13, 2022

View reviewed changes

BULK-22, datastax#398: Handle equivalent S3 credential queries

e0682eb

adutra added this to the 1.10.0 milestone May 27, 2022

Merge branch '1.x' into BULK-22-398-support-reading-s3-urls

1cc9bd4

weideng1 modified the milestones: 1.10.0, 1.11.0 Aug 12, 2022

Merge branch '1.x' into BULK-22-398-support-reading-s3-urls

8239cc8

adutra mentioned this pull request Nov 29, 2022

Support URL files with up to millions of lines #457

Open

adutra approved these changes Jan 31, 2023

View reviewed changes

weideng1 merged commit 48faf76 into datastax:1.x Jan 31, 2023

DavidTaylorCengage deleted the BULK-22-398-support-reading-s3-urls branch January 31, 2023 16:41

DavidTaylorCengage mentioned this pull request Jun 22, 2023

Loading from AWS S3 large file gives "Required array length is too large" error #478

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BULK-22, #398: Support reading s3 urls #399

BULK-22, #398: Support reading s3 urls #399

DavidTaylorCengage commented Feb 8, 2022 •

edited

Loading

DavidTaylorCengage commented Feb 10, 2022

adutra commented Feb 11, 2022

DavidTaylorCengage commented Feb 11, 2022

adutra commented Feb 18, 2022

adutra commented Feb 18, 2022

DavidTaylorCengage commented Feb 18, 2022

adutra commented Feb 19, 2022

DavidTaylorCengage commented Feb 21, 2022

adutra commented Feb 25, 2022

adutra commented Mar 8, 2022

DavidTaylorCengage commented Mar 8, 2022

adutra commented Mar 9, 2022

DavidTaylorCengage commented Mar 11, 2022

ErickRamirezAU left a comment •

edited

Loading

ErickRamirezAU Mar 16, 2022

DavidTaylorCengage Mar 16, 2022

ErickRamirezAU Mar 16, 2022

ErickRamirezAU Mar 16, 2022

ErickRamirezAU Mar 16, 2022

DavidTaylorCengage commented Mar 17, 2022

adutra commented Apr 1, 2022

DavidTaylorCengage commented Apr 1, 2022

DavidTaylorCengage commented Aug 25, 2022

DavidTaylorCengage commented Nov 17, 2022

adutra commented Nov 29, 2022

adutra commented Nov 29, 2022 •

edited

Loading


		#### --s3.clientCacheSize<br />--dsbulk.s3.clientCacheSize _<number>_

		The size (count) of the S3Client cache. Since each S3 URL

BULK-22, #398: Support reading s3 urls #399

BULK-22, #398: Support reading s3 urls #399

Conversation

DavidTaylorCengage commented Feb 8, 2022 • edited Loading

DavidTaylorCengage commented Feb 10, 2022

adutra commented Feb 11, 2022

DavidTaylorCengage commented Feb 11, 2022

adutra commented Feb 18, 2022

adutra commented Feb 18, 2022

DavidTaylorCengage commented Feb 18, 2022

adutra commented Feb 19, 2022

DavidTaylorCengage commented Feb 21, 2022

adutra commented Feb 25, 2022

adutra commented Mar 8, 2022

DavidTaylorCengage commented Mar 8, 2022

adutra commented Mar 9, 2022

DavidTaylorCengage commented Mar 11, 2022

ErickRamirezAU left a comment • edited Loading

Choose a reason for hiding this comment

ErickRamirezAU Mar 16, 2022

Choose a reason for hiding this comment

DavidTaylorCengage Mar 16, 2022

Choose a reason for hiding this comment

ErickRamirezAU Mar 16, 2022

Choose a reason for hiding this comment

ErickRamirezAU Mar 16, 2022

Choose a reason for hiding this comment

ErickRamirezAU Mar 16, 2022

Choose a reason for hiding this comment

DavidTaylorCengage commented Mar 17, 2022

adutra commented Apr 1, 2022

DavidTaylorCengage commented Apr 1, 2022

DavidTaylorCengage commented Aug 25, 2022

DavidTaylorCengage commented Nov 17, 2022

adutra commented Nov 29, 2022

adutra commented Nov 29, 2022 • edited Loading

DavidTaylorCengage commented Feb 8, 2022 •

edited

Loading

ErickRamirezAU left a comment •

edited

Loading

adutra commented Nov 29, 2022 •

edited

Loading