Skip to content
This repository was archived by the owner on Jan 12, 2026. It is now read-only.

Ion's table timeout code#41

Open
pcmoritz wants to merge 2 commits intomasterfrom
object-table-retry
Open

Ion's table timeout code#41
pcmoritz wants to merge 2 commits intomasterfrom
object-table-retry

Conversation

@pcmoritz
Copy link
Contributor

No description provided.

state/table.c Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe since there are a lot of arguments here and it is hard for a user to parse, we could separate out the ones that only have to do with failures into their own struct argument (this would probably be retry_count, timeout, fail_cb). We could also have a version of object_table_lookup that specifies default values for these arguments.

state/table.c Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to call retry_cb once before exiting this function? It seems like every time we would want to set a table callback, we would also want to call it once initially.

state/table.c Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way we can make sure that fail_cb won't get called if done_cb has already been called? Also, a way to make sure that if we call done_cb, the callback in outstanding_callbacks will get cleaned up by destroy_table_callback.

We should probably also test for it. :) I think you test whether fail_cb will get called for the object table right now, but not the others. It would be good to also test whether outstanding_callbacks gets updated the way we think it is.

state/redis.c Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should remove the timer here to prevent the failure callback from being called (see comment in state/table.c)

state/redis.c Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should remove the timer here to prevent the failure callback from being called (see comment in state/table.c)

state/table.c Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if we malloc something later on that gets allocated the same pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as you pointed out, one solution is to use the timer_id to distinguish (it is unique)

state/redis.c Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the callback in outstanding_callbacks will get cleaned up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add some documentation about what each test is testing for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, I think this is really only a test for the object table timeout, not generic timeouts. This goes back to the other comment in state/table.c about making sure we clean up everything properly if done_cb gets called. Currently, the tables seem to do different things once they've called done_cb.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should check in all cases that outstanding_cbs is cleaned up properly.

@istoica istoica force-pushed the object-table-retry branch from a1c1b13 to 92619c1 Compare October 21, 2016 01:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants