Skip to content

Conversation

@cameronlee314
Copy link
Contributor

@cameronlee314 cameronlee314 commented Apr 26, 2021

Issues: Some code was added for SEP-24 some time ago (#1172, #1173), but we are not moving forward with SEP-24 because it does not cleanly handle certain use cases. Since we don't need this code, it should get removed.

Changes:

  1. Removed unused flows related to job coordinator dependency isolation.
  2. Removed unused classloader separation utils.

Tests:
./bin/integration-tests.sh /tmp/samza-tests yarn-integration-tests --nopassword passed

API changes and usage/upgrade instructions:
Removed some configs and environment variables related to split deployment, but the feature wasn't complete, so those shouldn't be used by Samza jobs anyways.

Copy link
Contributor

@Sanil15 Sanil15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, I see in the testing section that you ran the yarn-integration tests, please ensure that still functional and up to date, I have personally never used it. I just deploy a samza-hello-samza app with a snapshot version.

IIRC was the code not config controlled ? If yes please cleanup any relevant markdowns / docs

@cameronlee314
Copy link
Contributor Author

Looks good overall, I see in the testing section that you ran the yarn-integration tests, please ensure that still functional and up to date, I have personally never used it. I just deploy a samza-hello-samza app with a snapshot version.

IIRC was the code not config controlled ? If yes please cleanup any relevant markdowns / docs

I think I did need to update a version locally for the integration tests, but they should otherwise be up to date. I would suggest trying them out, since then you don't need to manually verify anything as long as they report "success".
The code was config controlled by job.split.deployment.enabled, but since the impl wasn't completed, I don't think any docs had been modified to add that yet. I couldn't find any references to the config in the repo.

@cameronlee314 cameronlee314 merged commit de067c4 into apache:master Apr 28, 2021
@cameronlee314 cameronlee314 deleted the remove_split_deployment branch November 17, 2021 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants