Skip to content

Make sure cursor is a keyword argument#35

Merged
GustavoCaso merged 1 commit into
masterfrom
make-sure-cursor-keyword-argument-is-present-in-build_enumerator-method
Jul 2, 2019
Merged

Make sure cursor is a keyword argument#35
GustavoCaso merged 1 commit into
masterfrom
make-sure-cursor-keyword-argument-is-present-in-build_enumerator-method

Conversation

@GustavoCaso

@GustavoCaso GustavoCaso commented Jun 7, 2019

Copy link
Copy Markdown
Contributor

We found cases of people using cursor argument, not as a keyword argument.

This can lead to unexpected behaviour.

Discourse ticket

This change introduces an extra level of checking so when running the
job, we make sure the signature of build_enumerator has the cursor keyword
argument

@Shopify/job-patterns

@GustavoCaso GustavoCaso force-pushed the make-sure-cursor-keyword-argument-is-present-in-build_enumerator-method branch 2 times, most recently from cf95f9b to 4ffd907 Compare June 16, 2019 22:08
@GustavoCaso GustavoCaso changed the title [WIP] Make sure cursor is a keyword argument Make sure cursor is a keyword argument Jun 18, 2019
@GustavoCaso GustavoCaso requested review from BoGs and moechaieb June 18, 2019 10:53
def valid_cursor_parameter?(parameters)
# this condition is when people use the splat operator.
# def build_enumerator(*)
return true if parameters == [[:rest]]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

TIL

# this condition is when people use the splat operator.
# def build_enumerator(*)
return true if parameters == [[:rest]]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be slightly simpler?

    parameters.each do |parameter_type, parameter_name|
      next unless parameter_name == :cursor
      return true if parameter_type == :keyreq
    end
    false
  end

Comment thread test/unit/iteration_test.rb Outdated
work_one_job
end

def test_jobs_that_do_not_define_build_enumerator_or_each_iteration_will_not_raise

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

each_iteration_will_not_raise but the test has an assert_raises(ArgumentError)

@GustavoCaso GustavoCaso force-pushed the make-sure-cursor-keyword-argument-is-present-in-build_enumerator-method branch from 4ffd907 to c572cfb Compare June 18, 2019 13:21

@kirs kirs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for making it a better experience!


unless respond_to?(:build_enumerator, true)
if respond_to?(:build_enumerator, true)
parameters = method(:build_enumerator).parameters

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not strictly related to your PR or needs addressing, but it's somewhat pointless to check for arguments and the method signature on every perform of a job. Once a job class has been validated, we know it implements methods right. This makes me believe that we should be skipping assert_implements_methods! for classes that has already been validated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is a great point, we could have a local cache of checked classes and avoid having to call assert_implements_methods! unnecessary. I will explore this on another PR.

Comment thread lib/job-iteration/iteration.rb Outdated
parameters = method(:build_enumerator).parameters
unless valid_cursor_parameter?(parameters)
raise ArgumentError, "Iteration job (#{self.class}) #build_enumerator " \
"expects the keyword argument `cursor` please make sure that this condition is met"

@kirs kirs Jul 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpick: please make sure that this condition is met may not be the language that's typically used in exception messages. "expects the keyword argument cursor" is probably enough for developers to understand that's a condition that they have to satisfy :)

@kirs

kirs commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

Looks like method(:build_enumerator).parameters doesn't carry much performance overhead:

require 'benchmark/ips'

def build_enumerator
end

Benchmark.ips do |x|
  x.config(:time => 3, :warmup => 1)

  x.report("respond_to") do
    respond_to?(:build_enumerator)
  end
  x.report("respond_to + parameters") do
    respond_to?(:build_enumerator) && method(:build_enumerator).parameters
  end

  x.compare!
end
Warming up --------------------------------------
          respond_to   270.254k i/100ms
respond_to + parameters
                       264.253k i/100ms
Calculating -------------------------------------
          respond_to      8.025M (± 2.1%) i/s -     24.323M in   3.032362s
respond_to + parameters
                          8.136M (± 3.2%) i/s -     24.576M in   3.024205s

Comparison:
respond_to + parameters:  8135920.7 i/s
          respond_to:  8024793.9 i/s - same-ish: difference falls within error

a keyword argument.

This can lead to unxpected behaviour.

This change introiduce an extra level of checking so when running the
job, we make sure the signature of build_enumerator has the `cursor` keyword
argument
@GustavoCaso GustavoCaso force-pushed the make-sure-cursor-keyword-argument-is-present-in-build_enumerator-method branch from c572cfb to bc02e96 Compare July 2, 2019 14:41
@GustavoCaso GustavoCaso merged commit 8612cab into master Jul 2, 2019
@GustavoCaso GustavoCaso deleted the make-sure-cursor-keyword-argument-is-present-in-build_enumerator-method branch July 2, 2019 15:41
@GustavoCaso GustavoCaso temporarily deployed to rubygems July 17, 2019 13:44 Inactive
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.

4 participants