Refactor aws dynamodb streams scaler

[dttung2905]

Refactor aws dynamodb streams scaler.

Checklist

• I have verified that my change is according to the deprecations & breaking changes policy
• Changelog has been updated and is aligned with our changelog requirements
• Commits are signed with Developer Certificate of Origin (DCO - learn more)

Relates to #5797

[dttung2905]

@wozniakjan I'm leaving these out for now because the error logic handling is abit different in thise case. Instead of letting it failed on invalid input, this code just throws error and fallback to the default value

keda/pkg/scalers/aws_dynamodb_streams_scaler.go

Lines 97 to 114 in 66d4c95

	if val, ok := config.TriggerMetadata["shardCount"]; ok && val != "" {
	shardCount, err := strconv.ParseInt(val, 10, 64)
	if err != nil {
	meta.targetShardCount = defaultTargetDBStreamsShardCount
	logger.Error(err, "error parsing dyanmodb stream metadata shardCount, using default %n", defaultTargetDBStreamsShardCount)
	} else {
	meta.targetShardCount = shardCount
	}
	}
	if val, ok := config.TriggerMetadata["activationShardCount"]; ok && val != "" {
	shardCount, err := strconv.ParseInt(val, 10, 64)
	if err != nil {
	meta.activationTargetShardCount = defaultActivationTargetDBStreamsShardCount
	logger.Error(err, "error parsing dyanmodb stream metadata activationTargetShardCount, using default %n", defaultActivationTargetDBStreamsShardCount)
	} else {
	meta.activationTargetShardCount = shardCount
	}
	}

[wozniakjan]

thanks for pointing this out, I'm ok with this. But if you and other @kedacore/keda-core-contributors are willing to make an exception with a breaking change here, I'd like to advocate for a throwing error here. Imho it leads to overall better UX after the initial surprise that something that used to work (incorrectly) no longer does.

[zroubalik]

Hmm, I have no idea why did we allow this in a first place :) I don't like this kind of unexpected defaults. I am inclined in changing the behavior to error here.

WDYT @kedacore/keda-contributors ?

[JorTurFer]

Is something undocumented a breaking change? IMHO it depends on the time that has been there, if it's something recent we can change it, but this has been there for 2 years, so we should keep it IMO

[dttung2905]

Hmm how about we add a deprecation message logic into this current line. After the next two release, we then deprecate it by this PR? Wdyt?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[JorTurFer]

I guess that this isn't stale

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed due to inactivity.