Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

The sandboxed testing environment cannot use AWS #113

Closed
julienrf opened this issue Mar 12, 2024 · 7 comments · Fixed by #208
Closed

The sandboxed testing environment cannot use AWS #113

julienrf opened this issue Mar 12, 2024 · 7 comments · Fixed by #208
Assignees
@julienrf
Labels
enhancement New feature or request

Comments

@julienrf
Copy link
Collaborator

julienrf commented Mar 12, 2024 •
edited
Loading

In #107 we introduced a testing infrastructure that allows us to test several migration scenarios. Unfortunately, the streamChanges feature uses the spark-kinesis module under the hood and this module performs calls to the real AWS servers instead of using the containerized service.

Solutions to this problem could be to either fix the code of spark-kinesis to stay in the sandbox environment (this is a known issue, see localstack/localstack#677 and https://issues.apache.org/jira/browse/SPARK-27950), or to use something else than spark-kinesis.

Related:
@julienrf julienrf mentioned this issue Mar 12, 2024
Merged
julienrf added a commit to julienrf/scylla-migrator that referenced this issue Mar 13, 2024
@julienrf
Make streaming work
359a0e1 
Saving the initial data to the target database works, but saving additional changes fails with an exception “org.apache.hadoop.io.Text is not Serializable”.

I do not fully understand the difference between the initial data and the streamed changes that causes the error to happen. The only difference is that the initial data is read by the connector, whereas the streamed changes are created by us. I changed the way we create it to mix-in the `Serializable` interface in it.

This change allowed me to successfully run a data migration with stream changes enabled. Such a scenario can not be added to our test suite though because KCL only works with the real AWS servers (see scylladb#113)
julienrf added a commit to julienrf/scylla-migrator that referenced this issue Mar 13, 2024
@julienrf
Fix streaming issue
afcbfd8 
The RDD keys are not serializable, which can fail some RDD operations.

We create the RDD element keys _after_ repartitioning to avoid them being serialized across partitions.

This change allowed me to successfully run a data migration with stream changes enabled. Such a scenario can not be added to our test suite though because KCL only works with the real AWS servers (see scylladb#113)
julienrf added a commit to julienrf/scylla-migrator that referenced this issue Apr 16, 2024
@julienrf
Replace our fork of spark-kinesis with our adapted copy of the releva…
6a3403e 
…nt classes.

We adapted the `KinesisReceiver` and its related classes to work with DynamoDB Streams, and we renamed it into `KinesisDynamoDBReceiver`. These classes are based on the code from the original `spark-kinesis-asl` module with some slight modifications based on the following resources:

- https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Streams.KCLAdapter.Walkthrough.CompleteProgram.html
- https://medium.com/@ravi72munde/using-spark-streaming-with-dynamodb-d325b9a73c79
- and our previous fork implementation

As a result, instead of maintaining a complete fork of `spark-kinesis-asl`, we only maintain a copy of the relevant classes, which should result in much faster build times (especially in the CI).

It is still not possible to test the streaming feature locally (thus not in the CI either), see scylladb#113. These changes were tested with my actual AWS account.
julienrf added a commit to julienrf/scylla-migrator that referenced this issue Apr 16, 2024
@julienrf
Replace our fork of spark-kinesis with our adapted copy of the releva…
d1bf60e 
…nt classes.

We adapted the `KinesisReceiver` and its related classes to work with DynamoDB Streams, and we renamed it into `KinesisDynamoDBReceiver`. These classes are based on the code from the original `spark-kinesis-asl` module with some slight modifications based on the following resources:

- https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Streams.KCLAdapter.Walkthrough.CompleteProgram.html
- https://medium.com/@ravi72munde/using-spark-streaming-with-dynamodb-d325b9a73c79
- and our previous fork implementation

As a result, instead of maintaining a complete fork of `spark-kinesis-asl`, we only maintain a copy of the relevant classes, which should result in much faster build times (especially in the CI).

It is still not possible to test the streaming feature locally (thus not in the CI either), see scylladb#113. These changes were tested with my actual AWS account.

Fixes scylladb#119
@julienrf julienrf mentioned this issue Apr 18, 2024
Merged
julienrf added a commit to julienrf/scylla-migrator that referenced this issue Apr 26, 2024
@julienrf
Replace our fork of spark-kinesis with our adapted copy of the releva…
a76cb48 
…nt classes.

We adapted the `KinesisReceiver` and its related classes to work with DynamoDB Streams, and we renamed it into `KinesisDynamoDBReceiver`. These classes are based on the code from the original `spark-kinesis-asl` module with some slight modifications based on the following resources:

- https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Streams.KCLAdapter.Walkthrough.CompleteProgram.html
- https://medium.com/@ravi72munde/using-spark-streaming-with-dynamodb-d325b9a73c79
- and our previous fork implementation

As a result, instead of maintaining a complete fork of `spark-kinesis-asl`, we only maintain a copy of the relevant classes, which should result in much faster build times (especially in the CI).

It is still not possible to test the streaming feature locally (thus not in the CI either), see scylladb#113. These changes were tested with my actual AWS account.

Fixes scylladb#119
@julienrf julienrf mentioned this issue Jun 25, 2024
Merged
@julienrf julienrf self-assigned this Jun 25, 2024
@julienrf julienrf added the enhancement New feature or request label Jul 10, 2024
@julienrf
Copy link
Collaborator Author

Commenting on this issue instead of creating a new one because this is related to the testing infrastructure.

Currently, our testing infrastructure recreates the AWS stack (S3 and DynamoDB) in Docker containers. This works okay but comes with limitations:

  • we cannot test the streamChanges feature because that feature has hard-coded dependency on the real AWS (we cannot customize the endpoint used by some clients)
  • some authentication scenarios are hard to test properly: ultimately, we want to test that our authentication logic works with the real AWS, so there is no point in mocking AWS in a Docker container
  • our infrastructure does not allow us to perform benchmarks to ensure that we do not introduce performance regressions

While the first point could be (and should be, ideally) fixed by removing the hard-coded dependency on AWS, to address the second point we have no choice but having tests that use the real AWS. And, in practice, to fix the first point we would have to change things in our copy of the spark-kinesis project, which is undesirable (it is better to keep our copy as close as possible to the original so that we can merge the upstream improvements into our copy).

I believe those points motivate the need for having tests that use the real AWS instead of a containerized implementation of AWS. Except for the benchmarks, such tests should not be expensive because they would not consume a lot of bandwidth.

I propose the following course of action:

  • create AWS credentials that we can use for creating/deleting DynamoDB tables on the real AWS
  • create a new test module that tests the supported AWS authentication scenarios and the streaming feature
  • create a new GitHub workflow triggered by an explicit comment such as “Test on AWS” that runs the new test module
  • create a new test module that tests the migration throughput between real AWS (or real Apache Cassandra) and real ScyllaDB, and create a new GitHub workflow triggered by an explicit comment such as “Test performance” to run the module.

@julienrf julienrf changed the title Unable to test the streamChanges feature in a sandboxed environment The sandboxed testing environment cannot use AWS Jul 13, 2024
@guy9
Copy link
Collaborator

guy9 commented Jul 22, 2024

Thanks @julienrf, @tzach please have a look

@guy9
Copy link
Collaborator

guy9 commented Aug 15, 2024

@julienrf please proceed with your suggestion.
@tzach if you have any objections please let us know.

@tzach
Copy link

tzach commented Aug 15, 2024

I'm worried that using AWS for testing in a public repo may lead to abuse.

@julienrf
Copy link
Collaborator Author

Regarding this point:

  • we can trigger the workflow only under some conditions: e.g. one of the maintainers posts a comment “Test on AWS” on the PR
  • by default, GitHub does not automatically run workflows when someone who is not a maintainer submits a PR

@tzach
Copy link

tzach commented Aug 15, 2024

OK, if this is just for maintainers I'm good with that.
In other words: run on marge to master, not for every PR.

julienrf added a commit to julienrf/scylla-migrator that referenced this issue Aug 23, 2024
@julienrf
Apply the AlternatorEndpointProvider only on custom endpoints
6b0d047 
The previous implementation was failing when used on AWS DynamoDB because `conf.get(DynamoDBConstants.ENDPOINT)` was `null`.

This was not caught by our tests because our tests always use a custom endpoint (see scylladb#113)
@julienrf julienrf mentioned this issue Aug 23, 2024
Merged
julienrf added a commit that referenced this issue Aug 23, 2024
@julienrf
Apply the AlternatorEndpointProvider only on custom endpoints
aa000e5 
The previous implementation was failing when used on AWS DynamoDB because `conf.get(DynamoDBConstants.ENDPOINT)` was `null`.

This was not caught by our tests because our tests always use a custom endpoint (see #113)
This was referenced Aug 26, 2024
Closed
Merged
Open
@julienrf
Copy link
Collaborator Author

#208 is addressing the first part of the action plan. I’ve created #210 to track the remaining steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@julienrf julienrf

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add tests that access AWS julienrf/scylla-migrator
3 participants
@tzach @julienrf @guy9