[SERVER-5292] Patch for SERVER-4333 for 2.0.x Created: 12/Mar/12 Updated: 11/Jul/16 Resolved: 22/Mar/12 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Replication |
| Affects Version/s: | 2.0.3, 2.0.4 |
| Fix Version/s: | 2.1.1 |
| Type: | Bug | Priority: | Critical - P2 |
| Reporter: | Will Koffel | Assignee: | Eric Milkie |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Environment: |
Amazon Linux |
||
| Attachments: |
|
||||||||||||
| Issue Links: |
|
||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||
| Participants: | |||||||||||||
| Description |
|
I'm critically blocked on getting our sharded system live until I can differentiate between true user-initiated INSERTs and balancer-driven inserts. Fortunately, Milkie is "the man", and has fixed this. Unfortunately, it won't be in stable until 2.2. I've created a patch, attached here that should apply these changes to 2.0.3. But when I compiled the patch, built my own RPM, and ran it, I didn't see the change. It definitely is running the new 2.0.3 version (I had 2.0.2 on the machine before). Milkie, or someone else, is there any other "trick" to getting this behavior to trigger? Would be a huge win for me, this is literally the last blocker on a major rollout. Mongo is ready, but without this, I can't drive our analytics DB properly, so can't go live. |
| Comments |
| Comment by Scott Hernandez (Inactive) [ 22/Mar/12 ] |
|
As we don't backport new features we will provide a squashed patch for 2.0.4 in this special case. |
| Comment by Eric Milkie [ 22/Mar/12 ] |
|
Commit has been pushed. Follow |
| Comment by Eric Milkie [ 16/Mar/12 ] |
|
Hi Will, Sorry I haven't gotten back to you yet. I coded up a patch for this issue on the plane yesterday and tested it out; everything looks good. However, I can't push the change yet because we're in a bit of a mini-code freeze while we sort out some unit testing issues. You've probably made the same changes I did. I just went through and added fromMigrate to the parameter list for all the logOp() functions in update.cpp (for completeness). This change will definitely be in the final 2.2.0 release. Thanks for bringing this to our attention! I'll leave this open until the change gets pushed into the master 2.1.1 branch. |
| Comment by Will Koffel [ 14/Mar/12 ] |
|
Okay, so here's my take at the (2) lines to change. I'm sure this isn't the optimal fix, but in my testing, it causes both deletes and inserts to show with the fromMigrate flag. So hopefully good enough for my purposes until a final fix is in place. In db/ops/update.cpp In addition, I haven't found that it's necessary to pass the flag through to _updateById(), as it doesn't get used. I'm stuck running with this patch for now so I can get my project released. Obviously I'll keep a close eye on this ticket, and watch for info I missed, as well as confirmation that it's working as expected for the 2.2 series releases. Thanks for engaging and acknowledging the issue quickly, it gave me the confidence to dive deeper. And sorry for not supplying proper patches or git pull requests. This was a bit of a guerrilla exercise here. |
| Comment by Will Koffel [ 14/Mar/12 ] |
|
Any love on this, Eric? Or should I dive in and work on it myself (the beauty and curse of open source!) I expect it's about a 3 line change...those elusive 3 lines haunted my confused dreams last night... |
| Comment by Will Koffel [ 13/Mar/12 ] |
|
Thanks so much Eric. Should I watch here or the github logs? If this won't get done right away, can you point me anywhere that I can look for the fix myself? I'm on a major deadline this week, so I'll be working on it in parallel today, but obviously you're a lot more equipped to get it right! Let me know if there's anything at all I can do to help. |
| Comment by Eric Milkie [ 13/Mar/12 ] |
|
You're right; the insert logging is broken. I'll fix it. – Posted from Bugbox for iPhone |
| Comment by Will Koffel [ 13/Mar/12 ] |
|
Hey guys, here's what I've discovered after getting a test environment set up... It appears that DELETE ops, ( {"op":"d"}) are getting the {"fromMigrate":true}flag set correctly. This makes sense. It happens in your patches (https://github.com/mongodb/mongo/commit/abc3e0cb35c4ab0e837936e04f378c1d963c849b) at db/dbhelpers.cpp line 241. HOWEVER, the corresponding INSERTs don't have any sign of the "fromMigrate" flag, they seem to have not been affected at all. Inside _updateById, although it takes the 'bool fromMigrate' final argument, it NEVER uses that argument anywhere. That seems to be a problem. If it didn't need the arg, why include it. My hunch is that it does need it, but the patch is incomplete. So unless I'm missing something (definitely possible), there might be a bug in your patch as well as my patch. Any thoughts? I get a bit lost inside that _updateById() function, so I can't tell where the actual logOp happens. This also might be a red-herring, since the op gets logged as an insert {"op":"i"}, and the only ops I see from _updateById() are UPDATE ops. So maybe the issue is further back, inside update.cpp line 1274 and 1284 both are suspect, as they log inserts, but don't include the fromMigrate param that was passed into that function. Should they? |
| Comment by Will Koffel [ 12/Mar/12 ] |
|
second patch file, forgot to include the /s/ subdir. |
| Comment by Will Koffel [ 12/Mar/12 ] |
|
Sorry, yes, I also did patch that. When I ran my diff, I did it just on the db/ subdir. But the d_migrate.cpp is also patched. Clearly that's where the actual work happens. I've attached that patch here now as well (sorry, I've been working in that dir, so there's a bunch of extraneous object files, etc.) |
| Comment by Eliot Horowitz (Inactive) [ 12/Mar/12 ] |
|
Looks like you're missing the patch to s/d_migrate.cpp that actually makes the change. |