Skip to content

Conversation

@dfangl
Copy link
Member

@dfangl dfangl commented Nov 15, 2023

Motivation

Our kinesis_connector.py is currently used at several locations in our code, and the usage of it is in principal valid.
However, it has some problems with threads not shutting down, processes kept alive indefinitely, and a lot of unused configuration options cluttering the code.

To continue using it, we want:

  • A clear shutdown of all processes and threads, with a single stop
  • A reduction in created processes and threads
  • Simpler code, by refactoring and removing unused configuration options

Changes

  • Enable new (-ext) Resource Provider for AWS::KinesisAnalytics::Application
  • Refactor kinesis connector, get rid of output file reader threads, proper shutdown, more class encapsulation, less configuration variable
  • Remove "environment" logic from aws stack
  • Add try/finally around docker container startup logic to prevent stuck threads waiting for a event forever.

@dfangl dfangl added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Nov 15, 2023
@github-actions
Copy link

github-actions bot commented Nov 15, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 5m 7s ⏱️
2 330 tests 2 030 ✔️ 300 💤 0
2 331 runs  2 030 ✔️ 301 💤 0

Results for commit 3c3be8d.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Nov 15, 2023

Coverage Status

coverage: 84.024% (+0.01%) from 84.013%
when pulling 3c3be8d on cleanup-kinesis-connector
into 177773b on master.

@dominikschubert dominikschubert added this to the Playground milestone Nov 15, 2023
@dfangl dfangl force-pushed the cleanup-kinesis-connector branch from dec5f9e to f71f6f8 Compare November 16, 2023 11:39
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes here are only removal of the shard_id parameter of process records, and migration to the cleanups fixture instead of try: finally:. The changed lines within that block are just indentation changes.

@dfangl dfangl force-pushed the cleanup-kinesis-connector branch from 701e19a to c8218c1 Compare November 17, 2023 14:36
@dfangl dfangl force-pushed the cleanup-kinesis-connector branch from 3051a44 to cf4317a Compare November 20, 2023 13:19
@dfangl dfangl added semver: patch Non-breaking changes which can be included in patch releases and removed semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Nov 21, 2023
@dfangl
Copy link
Member Author

dfangl commented Nov 21, 2023

Discussed with alex, and decided to go with patch for now. Up for discussion though, if a reviewer thinks it should be minor :)

@dfangl dfangl modified the milestones: Playground, 3.0.1 Nov 21, 2023
Copy link
Contributor

@simonrw simonrw left a comment

Choose a reason for hiding this comment

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

I have no strong opinions on the kinesis connector refactoring, but I think this is a step in the right direction. Anything to properly manage the lifetimes of the kinesis connector resources is a good thing!

edit: happy with semver patch

@dfangl dfangl merged commit dcc4a7b into master Nov 22, 2023
@dfangl dfangl deleted the cleanup-kinesis-connector branch November 22, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants