Add support for selectively choosing deduplication keys. #102

kyto64 · 2023-09-26T05:32:54Z

Description of changes:

Previously, when using ContentBasedDeduplication, only job_id was excluded from the deduplication key. However, depending on the requirements of Rails applications, it may be necessary to exclude keys other than job_id as well.

I have addressed this issue by allowing the deduplication key to be customized in the configuration or within each job class.
Please note that, by default, only job_id is excluded.

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

mullermp

Thanks for opening a pull request. I think this is a valid feature to consider.

mullermp · 2023-09-26T18:40:54Z

lib/active_job/queue_adapters/amazon_sqs_adapter.rb

+        dedup_keys = job.deduplication_keys if job.respond_to?(:deduplication_keys)
+        dedup_keys ||= Aws::Rails::SqsActiveJob.config.deduplication_keys.map(&:to_s)


I think it makes sense to use symbol throughout the method (including for job_id), and then convert to string when doing body.slice. It would eliminate to_s here and in deduplicate_with.

Use to_s within each definition to uniquely add job_id to keys within the deduplicate_without method and excluded_deduplication_keys getter definitions.
Anyway, I agree with your ideas and would be happy to hear any other good ideas.

mullermp · 2023-09-26T18:43:41Z

lib/aws/rails/sqs_active_job/configuration.rb

        #   client will be created if none is provided.
+        #
+        # @option options [Array] :deduplication_keys ([])


It'd be nice to convey the default here (all properties except job id) since it is not actually an empty array, or maybe the configuration should be changed to be an exclude list instead of an override (which job id can be hard coded appended to), whichever has better customer experience.

I'd personally lean towards an exclude list instead of include list. That way the default doesn't need to change if other fields are added in the future. And in general, I'd imagine most use cases would want to just exclude 1 or 2 additional fields?

Revised with feedback from alextwoods :)

mullermp · 2023-09-26T18:51:59Z

lib/aws/rails/sqs_active_job/deduplication.rb

+module Aws
+  module Rails
+    module SqsActiveJob
+      extend ActiveSupport::Concern


Good use of concern here. Are there any other SQS active job related examples in this gem we can clean up as such?

mullermp · 2023-09-26T18:53:12Z

test/active_job/queue_adapters/amazon_sqs_adapter_test.rb

+              message_body: instance_of(String),
+              message_attributes: instance_of(Hash),
+              message_group_id: Aws::Rails::SqsActiveJob.config.message_group_id,
+              message_deduplication_id: instance_of(String)


Can we try to assert the default and the override deduplication_id below to ensure that the configuration is working as expected?

I fixed this and added some specs.

mullermp · 2023-09-26T18:56:00Z

It looks like test failures are related to MiniTest somehow. The JRuby 9.4 failures appear to be either some deprecation or bug.

alextwoods

Overall looks good and I think is a useful feature - thanks for submitting!

alextwoods · 2023-09-26T22:16:46Z

lib/active_job/queue_adapters/amazon_sqs_adapter.rb

+
+        ignored_dedup_key = 'job_id'
+
+        if dedup_keys.include?(ignored_dedup_key)


It might be better to check for an invalid key upfront, when its set rather then every time its used (raise the error where its being made) - I think you could do that in the setter for deduplication_keys.

lib/active_job/queue_adapters/amazon_sqs_adapter.rb

alextwoods · 2023-09-26T22:19:48Z

lib/aws/rails/sqs_active_job/configuration.rb

        #   client will be created if none is provided.
+        #
+        # @option options [Array] :deduplication_keys ([])


I'd personally lean towards an exclude list instead of include list. That way the default doesn't need to change if other fields are added in the future. And in general, I'd imagine most use cases would want to just exclude 1 or 2 additional fields?

…eys to adopt, one can choose which keys to exclude. That way the default doesn't need to change if other fields are added in the future.

mullermp

Nice! Just some minor comments and I'll take a look at fixing CI ..

mullermp · 2023-09-27T19:27:14Z

lib/aws/rails/sqs_active_job/configuration.rb

        #   client will be created if none is provided.
+        #
+        # @option options [Array] :excluded_deduplication_keys ('job_id')


Default should be (['job_id']) since it's Array

mullermp · 2023-09-27T19:29:48Z

test/active_job/queue_adapters/amazon_sqs_adapter_test.rb

+                  message_body: instance_of(String),
+                  message_attributes: instance_of(Hash),
+                  message_group_id: Aws::Rails::SqsActiveJob.config.message_group_id,
+                  message_deduplication_id: instance_of(String)


Is there a way to strongly assert that message_deduplication_id does not contain job_class and queue_name here? Maybe an implementation detail but we could assert Digest::SHA256.hexdigest or Aws::Json.dump doesn't receive them.

mullermp · 2023-09-27T21:21:17Z

The test failures in main should be fixed.

mullermp · 2023-09-27T22:35:21Z

I pushed a change to main that configured and ran rubocop on everything, as well as bump the minimum rails version to 2.3. Please check to see if main merged in correctly and didn't stomp on any of your changes.

mullermp · 2023-09-27T22:36:13Z

Please also fix your rubocop errors (new CI I've added)

Strongly assert that message_deduplication_id does not contain keys to exclude.

kyto64 · 2023-09-28T01:41:12Z

To @mullermp @alextwoods

Confirmation about Rubocop:

> bundle e rubocop -E -S 
===>
test/active_job/queue_adapters/amazon_sqs_adapter_test.rb:7:3: C: Metrics/ModuleLength: Module has too many lines. [144/112]
  module QueueAdapters ...
  ^^^^^^^^^^^^^^^^^^^^
test/active_job/queue_adapters/amazon_sqs_adapter_test.rb:8:5: C: Metrics/BlockLength: Block has too many lines. [142/110]
    describe AmazonSqsAdapter do ...

Rubocop also limits the number of lines in the test files, but this may be an unnecessary calibration due to the functionality of RSpec. RSpec is written in a dedicated DSL, which inevitably increases the number of lines per block.

If you agree with this, I will add the following code to .rubocop.yml to avoid the warning:

# .rubocop.yml

Metrics/BlockLength:
  Exclude:
    - 'test/**/*.rb'

Metrics/ModuleLength:
  Exclude:
    - 'test/**/*.rb'

kyto64 · 2023-09-28T02:16:25Z

Note that Rubocop CI still detects errors unless #102 (comment) is applied or fixed individually.

mullermp · 2023-09-28T03:02:25Z

Yeah, it's fine to add to to-do or global ignore.

kyto64 · 2023-09-28T03:29:18Z

#102 (comment)
Added it to global ignore :)

mullermp · 2023-09-28T18:53:46Z

Thanks for your contribution! I will get this released tomorrow.

mullermp · 2023-09-28T21:30:45Z

Actually, I was able to do this today, please check to see if it works for you!

kyto64 · 2023-09-29T00:08:11Z

Thanks a ton!

Add support for selectively choosing deduplication keys.

cadd4a7

mullermp reviewed Sep 26, 2023

View reviewed changes

mullermp requested review from mullermp and alextwoods September 26, 2023 18:56

alextwoods reviewed Sep 26, 2023

View reviewed changes

kyto64 requested a review from alextwoods September 27, 2023 11:02

Fix to specify deduplication keys so that instead of choosing which k…

472630e

…eys to adopt, one can choose which keys to exclude. That way the default doesn't need to change if other fields are added in the future.

kyto64 force-pushed the add-dedup-keys-customization branch from c7e035a to 472630e Compare September 27, 2023 14:03

mullermp approved these changes Sep 27, 2023

View reviewed changes

Merge branch 'main' into add-dedup-keys-customization

15fdaa9

Merge branch 'main' into add-dedup-keys-customization

44481c9

Keito Kira added 2 commits September 28, 2023 08:38

Fix comments.

e1a5312

Fix specs.

9aa17c6

Strongly assert that message_deduplication_id does not contain keys to exclude.

kyto64 requested a review from mullermp September 28, 2023 01:50

Fix the code according to Rubocop.

a71a47f

Keito Kira added 2 commits September 28, 2023 12:22

Don't check block / module length in test/**/*.rb by Rubocop.

a2f03a1

Fix newline code to LF.

72cada8

mullermp approved these changes Sep 28, 2023

View reviewed changes

mullermp merged commit 925fbb5 into aws:main Sep 28, 2023
35 checks passed

kyto64 deleted the add-dedup-keys-customization branch September 29, 2023 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for selectively choosing deduplication keys. #102

Add support for selectively choosing deduplication keys. #102

kyto64 commented Sep 26, 2023 •

edited

mullermp left a comment

mullermp Sep 26, 2023

kyto64 Sep 27, 2023

mullermp Sep 26, 2023

alextwoods Sep 26, 2023

kyto64 Sep 27, 2023

mullermp Sep 26, 2023

mullermp Sep 26, 2023

kyto64 Sep 27, 2023 •

edited

mullermp commented Sep 26, 2023

alextwoods left a comment

alextwoods Sep 26, 2023

alextwoods Sep 26, 2023

mullermp left a comment

mullermp Sep 27, 2023

mullermp Sep 27, 2023

mullermp commented Sep 27, 2023

mullermp commented Sep 27, 2023

mullermp commented Sep 27, 2023

kyto64 commented Sep 28, 2023 •

edited

kyto64 commented Sep 28, 2023

mullermp commented Sep 28, 2023

kyto64 commented Sep 28, 2023

mullermp commented Sep 28, 2023

mullermp commented Sep 28, 2023

kyto64 commented Sep 29, 2023

		dedup_keys = job.deduplication_keys if job.respond_to?(:deduplication_keys)
		dedup_keys \|\|= Aws::Rails::SqsActiveJob.config.deduplication_keys.map(&:to_s)


		ignored_dedup_key = 'job_id'

		if dedup_keys.include?(ignored_dedup_key)

Add support for selectively choosing deduplication keys. #102

Add support for selectively choosing deduplication keys. #102

Conversation

kyto64 commented Sep 26, 2023 • edited

mullermp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyto64 Sep 27, 2023 • edited

Choose a reason for hiding this comment

mullermp commented Sep 26, 2023

alextwoods left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullermp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullermp commented Sep 27, 2023

mullermp commented Sep 27, 2023

mullermp commented Sep 27, 2023

kyto64 commented Sep 28, 2023 • edited

kyto64 commented Sep 28, 2023

mullermp commented Sep 28, 2023

kyto64 commented Sep 28, 2023

mullermp commented Sep 28, 2023

mullermp commented Sep 28, 2023

kyto64 commented Sep 29, 2023

kyto64 commented Sep 26, 2023 •

edited

kyto64 Sep 27, 2023 •

edited

kyto64 commented Sep 28, 2023 •

edited