Skip to content

Conversation

lizard-boy
Copy link

Motivation

Changes

maxhoheiser and others added 25 commits January 31, 2024 11:27
…#10143)

Co-authored-by: Benjamin Simon <benjh.simon@gmail.com>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request introduces new tests and enhancements for Kinesis Firehose functionality in LocalStack, focusing on resharding operations and multiple delivery streams.

  • Added tests for Firehose with Kinesis as source, validating behavior during resharding (split and merge) operations
  • Implemented new test for multiple Firehose delivery streams connected to a single Kinesis stream
  • Enhanced Firehose provider to support unique DynamoDB lease tables for each delivery stream
  • Improved Kinesis mock server log level handling for better consistency with LocalStack logging
  • Updated fixtures in conftest.py to support more flexible Firehose and Kinesis testing scenarios

30 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings

# publish metric only if CloudWatch service is available
if not is_api_enabled("cloudwatch"):
return
cw_client = connect_to(region_name=region_name).cloudwatch
cw_client = connect_to(aws_access_key_id=account_id, region_name=region_name).cloudwatch
Copy link

Choose a reason for hiding this comment

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

style: Consider using a consistent parameter order across functions (e.g., account_id before region_name)

@@ -3,16 +3,14 @@
"use strict";

const AWS = require("aws-sdk");
let dynamoDb;

var config = {};
Copy link

Choose a reason for hiding this comment

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

style: Consider using 'const' instead of 'var' for better scoping

@@ -3,16 +3,14 @@
"use strict";

const AWS = require("aws-sdk");
let dynamoDb;

var config = {};
if (process.env.AWS_ENDPOINT_URL) {
Copy link

Choose a reason for hiding this comment

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

style: Consider using const instead of var for config object

Comment on lines +107 to +108
shard_id = shard["ShardId"][-2:]
starting_hash_keys[shard_id] = shard["HashKeyRange"]["StartingHashKey"]
Copy link

Choose a reason for hiding this comment

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

logic: Extracting only the last 2 characters of ShardId may not be reliable for all shard naming conventions

Comment on lines +128 to +129
json_array_string = "[" + input_string.replace("}{", "},{") + "]"
message = json.loads(json_array_string)
Copy link

Choose a reason for hiding this comment

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

style: This JSON parsing method assumes a specific format. Consider using a more robust parsing approach

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.

9 participants