Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug about chunk response handling #1389

Merged
merged 1 commit into from
Dec 26, 2016
Merged

Conversation

tagomoris
Copy link
Member

fixes #1388.

v0.14.11 should be released after this change is merged for quick regression fix.
The changes between v0.14.10 release and this change will be introduced at v0.14.12.

@tagomoris tagomoris added bug Something isn't working v0.14 labels Dec 23, 2016
@tagomoris tagomoris self-assigned this Dec 23, 2016
@tagomoris tagomoris requested a review from repeatedly December 23, 2016 12:12
@tagomoris
Copy link
Member Author

@repeatedly could you review this?

@@ -214,22 +214,23 @@ def write(chunk)
select_a_healthy_node{|node| node.send_data(tag, chunk) }
end

ACKWaitingSockInfo = Struct.new(:sock, :chunk_id, :node, :time, :timeout) do
ACKWaitingSockInfo = Struct.new(:sock, :chunk_id, :chunk_id_base64, :node, :time, :timeout) do
Copy link
Member

Choose a reason for hiding this comment

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

Just one idea.
How about using def chunk_id_base64 instead of struct member?
It reduces additional member cost.

Copy link
Member Author

@tagomoris tagomoris Dec 24, 2016

Choose a reason for hiding this comment

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

It's not bad idea, but it looks better for me to add these methods to chunk:

  • unique_id_hex
  • unique_id_base64

These values are called many times (especially _hex for logging), and should be cached not to calculate 2 or more times.
How do you think about it?

Copy link
Member

Choose a reason for hiding this comment

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

Hm... it also seems good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another one: chunk.unique_id(:hex) (or :base64)

Copy link
Member

Choose a reason for hiding this comment

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

I like method approach, unique_id_hex, rathar than unique_id(:hex).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing this point makes many bad things (one is unreasonable SEGV in Travis) and doesn't bring so big happy.
I'll leave this as is.

@@ -178,7 +178,7 @@ def start
# But it should be overwritten by ack_response_timeout to rollback chunks after timeout
if @ack_response_timeout && @delayed_commit_timeout != @ack_response_timeout
log.info "delayed_commit_timeout is overwritten by ack_response_timeout"
Copy link
Member

Choose a reason for hiding this comment

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

OT. Adding why message is better for users to understand why delayed_commit_timeout is overwritten in out_forward.

@@ -178,7 +178,7 @@ def start
# But it should be overwritten by ack_response_timeout to rollback chunks after timeout
if @ack_response_timeout && @delayed_commit_timeout != @ack_response_timeout
Copy link
Member

Choose a reason for hiding this comment

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

If @delayed_commit_timeout and @ack_response_timeout are the same, @delayed_commit_timeout = @ack_response_timeout + 2 line is skipped. Is this okay?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, because these are configured intentionally by users, and don't cause problems for longer configured values.

Copy link
Member

Choose a reason for hiding this comment

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

I see.

@tagomoris tagomoris force-pushed the fix-bug-forward-ack-reponse branch from e295e70 to 0579cce Compare December 26, 2016 03:09
@tagomoris tagomoris merged commit 739e7db into master Dec 26, 2016
@tagomoris tagomoris deleted the fix-bug-forward-ack-reponse branch December 26, 2016 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v0.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

at-least-once acknowledgements are not processed
2 participants