Update Last Location. Fix #1169 #1616
Conversation
|
Shouldn't this include a revert of the previous revert? |
|
I am not sure I folllow what you mean. Sorry. |
I don't think so. He pulled out this one line from the previous change (which included too many other changes that broke the build). |
|
I am currently working on getting those other changes to build but this one should be good to go. |
|
I am in the process of running all the ITs. I want to make sure this change wasn't part of problem which forced us to revert. |
milleruntime
left a comment
milleruntime
left a comment
There was a problem hiding this comment.
I think this change is OK but I would like for the other build issues in 2.1 to get resolved first before merging.
|
I don't currently understand assignments to know what this patch fixes or why it fixes it. Can you add a brief comment here (so that way it can be included in the commit log) to explain what is being fixed, specifically, and why this fix works? Once we have that, and the ITs pass, I'd be okay with merging it for 2.1 (with that text included in the commit log message when it is merged). |
|
The idea behind this change is to guarantee lastLocation gets assigned to the correct location. In my testing, a cluster can fully balance but lastLocation still have its original value, of either null or the location the tablet was initially in. In these instances, if the cluster is taken down and back up, it would go back to its original unbalanced state. This issue was brought up by @ivakegg since the balancing for his larger data set took quite a bit of time to do the entire balancing process. This pull request will ideally alleviate that from happening. |
|
Thanks. I'm okay with this change... I just wish we had a better grasp on the intended (designed) role that last location played, as distinct from the current location. I don't think that's documented anywhere, and it makes it really hard to address issues in this area. |
|
Have all the ITs been run with this change? |
|
They were at the time but not recently. |
|
I ran the full ITs overnight and everything looks good. I'll merge this today. Thanks, @Manno15 . |
This reverts commit 1c0f8df. This changed the behavior of updating the LAST location field to update on assignment, rather than on writing data to DFS. The LAST location field should be updated when writing data to DFS, because it identifies the location where the tablet last wrote data, so that a balancing strategy can re-assign it to a location where at least some data is local.
|
Reverted due to conclusions realized in #1653 (comment) |
This reverts commit 1c0f8df. This changed the behavior of updating the LAST location field to update on assignment, rather than on writing data to DFS. The LAST location field should be updated when writing data to DFS, because it identifies the location where the tablet last wrote data, so that a balancing strategy can re-assign it to a location where at least some data is local.
This reverts commit 1c0f8df. This changed the behavior of updating the LAST location field to update on assignment, rather than on writing data to DFS. The LAST location field should be updated when writing data to DFS, because it identifies the location where the tablet last wrote data, so that a balancing strategy can re-assign it to a location where at least some data is local.
4 participants
This the one-line change leftover after the revert of pull request #1453. The rest of those changes will be tested more thoroughly and hopefully can be salvaged. In the meantime, this change fixes the issue without a bunch of the refactoring done in the previous pull request. I apologize for the delay.
I ran the entire IT suite, and I didn't see any relavent breakages from this change besides the one mentioned below.
Also, an AssertNull was removed from the MasterAssignmentIT. Let me know if that is acceptable or if I need to refactor that IT further.