Skip to content

Make create_table wait for table#8

Merged
eredi93 merged 2 commits intoeredi93:masterfrom
gordonbondon:create_table
Jul 23, 2018
Merged

Make create_table wait for table#8
eredi93 merged 2 commits intoeredi93:masterfrom
gordonbondon:create_table

Conversation

@gordonbondon
Copy link
Copy Markdown
Contributor

It takes some time to initialize DynamoDB table, and previously I was getting this errors on creation:

Aws::DynamoDB::Errors::ResourceInUseException: Attempt to change a resource which is still in use: Table lock_table_name is being created

or

Aws::DynamoDB::Errors::ResourceNotFoundException: Requested resource not found: Table: lock_table_name not found

Plus I made this call idempotent, allowing to make create_table call on existing tables.

Comment thread lib/marloss/store.rb Outdated
},
table_name: table
)
begin
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this method is becoming quite big 😬
can you split it into 3 smaller private methods that then gets called by the create_table method?

something like:

def create_table
  create_ddb_table
  wait_until_ddb_table_exists
  set_ddb_table_ttl
end

private def create_ddb_table
  client.create_table(
    ...
  )
rescue Aws::DynamoDB::Errors::ResourceInUseException => e
  case e.massage
  when "Table already exists: #{table}"
    Marloss.logger.warn("DynamoDB table #{table} already exists")
  else
    raise
  end
end

private def wait_until_ddb_table_exists
  client.wait_until(:table_exists, table_name: table) do |w|
    w.max_attempts = 10
    w.delay = 1
  end
rescue Aws::Waiters::Errors::WaiterFailed => e
  Marloss.logger.error("Failed waiting for initialization of table #{table}")
  raise
end

private def set_ddb_table_ttl
  client.update_time_to_live(
    ...
  )
rescue Aws::DynamoDB::Errors::ValidationException => e
  case e.message
  when "TimeToLive is already enabled"
    Marloss.logger.warn("TTL attribute is already configured for table #{table}")
  else
    raise
  end
end

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.

Easy!

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.

How about keeping create and wait in one method and splitting out ttl?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

that could cause confusion whether the TTL is required or not. I think exposing a simple method would create a better user experience

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.

sounds fair. I've added separate wait function

Comment thread README.md

# refresh the lock once
locker.refresh
locker.refresh_lock
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

👍

Comment thread lib/marloss/store.rb Outdated
end

begin
client.wait_until(:table_exists, table_name: table) do |w|
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

why not splitting the waiting to a separate method as I proposed?

Comment thread lib/marloss/store.rb Outdated
end

private def create_ddb_table
begin
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this begin is not necessary

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.

fixed

Copy link
Copy Markdown
Owner

@eredi93 eredi93 left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@eredi93
Copy link
Copy Markdown
Owner

eredi93 commented Jul 23, 2018

@gordonbondon Thanks you for getting this PR together! 🙇

@eredi93 eredi93 merged commit 0fdc34f into eredi93:master Jul 23, 2018
@gordonbondon gordonbondon deleted the create_table branch July 23, 2018 12:54
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.

2 participants