Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Speculative execution fixes #979

Merged
merged 6 commits into from
Aug 7, 2018
+28 −15
Merged

Speculative execution fixes #979

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e3c47c6
clarify
Dieterbe Aug 6, 2018
af8923b
implement speculation-threshold by its spec
Dieterbe Aug 6, 2018
8d06e78
assure ticker is always cleaned up
Dieterbe Aug 7, 2018
dbb9bf6
simpler way to compute percentReceived
Dieterbe Aug 7, 2018
6690299
doc fixes
Dieterbe Aug 7, 2018
3428869
no need for pendingResponses
Dieterbe Aug 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
simpler way to compute percentReceived
@Dieterbe
Dieterbe committed Aug 7, 2018
commit dbb9bf6623e1b6897f732bc92a7b3611216d91c4
2 changes: 1 addition & 1 deletion api/cluster.go
View file
Original file line number Diff line number Diff line change
@@ -390,7 +390,7 @@ func (s *Server) peerQuerySpeculative(ctx context.Context, data cluster.Traceabl

case <-tickChan:
// Check if it's time to speculate!
percentReceived := 1 - (float64(len(pendingResponses)) / float64(len(peerGroups)))
percentReceived := float64(len(receivedResponses)) / float64(len(peerGroups))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is pendingResponses still worth anything? It seems superfluous. The current uses are:

  1. To drive the main loop (for len(pendingResponses) > 0) which could be for len(receivedResponses) < len(peerGroups)
  2. To decide upon who to speculate which could loop on peerGroups and continue if in recievedResponses.

I'm not sure it's worth the extra allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

that's changing the logic, but it's actually making it correct, so that's good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the change in logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this particular line should not change logic. but the line below it does.
i explained in depth in the commit message
(i generally try to make commit messages explain everything)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that was my analysis as well 👍

if percentReceived >= speculationThreshold {
// kick off speculative queries to other members now
ticker.Stop()