Why are we not rolling back?
-
@boomzilla You fool, now your local instance will be broken as well! We could have migrated to your computer
Filed Under: BackUp-Strategies
-
@boomzilla said in Why are we not rolling back?:
FYI, I'm upgrading locally (been a long time since that happened, so there's more here than what happened in the latest TDWTF upgrade)...
Console output...
~/nodebb$ ./nodebb upgrade 1. Bringing base dependencies up to date... OK 2. Checking installed plugins for updates... OK A total of 1 package(s) can be upgraded: * nodebb-plugin-htmlcleaner (0.1.8 -> 0.1.11) Proceed with upgrade (y|n)? y Upgrading packages... OK 3. Updating NodeBB data store schema... [deprecated] `String.prototype.rtrim` is deprecated as of NodeBB v1.5; use `utils.rtrim` instead. 12/5 08:26:35 [2290] - warn: You have no mongo password setup! 12/5 08:26:35 [2290] - warn: [cache-buster] could not read cache buster: ENOENT: no such file or directory, open '/home/boomzilla/nodebb/build/cache-buster' Parsing upgrade scripts... OK | 28 script(s) found → [2015/12/23] Chat room hashes... skipped → [2015/12/15] Upgrading chats... skipped → [2016/2/25] Social: Post Sharing... skipped → [2015/12/23] Adding theme to active plugins sorted set... skipped → [2016/1/14] Creating user best post sorted sets... skipped → [2016/1/20] Creating users:notvalidated... skipped → [2016/1/23] Creating Global moderators group... skipped → [2016/5/28] Giving topics:read privs to any group that was previously allowed to Find & Access Category... skipped → [2016/4/14] Group title from settings to user profile... skipped → [2016/6/13] Store upvotes/downvotes separately... skipped → [2016/4/19] Users post count per tid... skipped → [2016/4/29] Dismiss flags from deleted topics... skipped → [2016/8/5] Removing best posts with negative scores... skipped → [2016/7/12] Giving upload privileges... skipped → [2016/9/22] Category recent tids... skipped → [2016/8/7] Granting edit/delete/delete topic on existing categories... skipped → [2016/10/8] Favourites to Bookmarks... skipped → [2016/10/14] Sorted sets for post replies... skipped → [2016/11/22] Update global and user language keys... skipped → [2016/11/25] Sorted set for pinned topics... skipped → [2017/2/28] Upgrading config urls to use assets route... OK → [2017/2/25] Update global and user sound settings... OK → [2017/4/16] Delete accidentally long-lived sessions... OK → [2017/4/14] Set default allowed file extensions... OK → [2017/3/22] Update moderation notes to zset... → [2017/3/22] Update moderation notes to zset... OK → [2017/2/27] New sorted set posts:votes... → [2017/2/27] New sorted set posts:votes... OK → [2016/12/7] Migrating flags to new schema... → [2016/12/7] Migrating flags to new schema... OK → [2017/4/26] Remove relative_path from uploaded profile cover urls... → [2017/4/26] Remove relative_path from uploaded profile cover urls... OK Upgrade complete! 12/5 08:33:57 [2290] - warn: You have no mongo password setup! 12/5 08:33:57 [2290] - info: [build] Building javascript 12/5 08:33:57 [2290] - info: [build] Building client-side CSS WARN: Output exceeds 32000 characters WARN: Output exceeds 32000 characters WARN: Output exceeds 32000 characters WARN: Output exceeds 32000 characters WARN: Output exceeds 32000 characters 12/5 08:34:02 [2290] - info: [build] clientCSS => Completed in 4.781s 12/5 08:34:02 [2290] - info: [build] Building admin control panel CSS 12/5 08:34:04 [2290] - info: [build] acpCSS => Completed in 2.303s 12/5 08:34:04 [2290] - info: [build] Building templates 12/5 08:34:04 [2290] - info: [build] tpl => Completed in 0.197s 12/5 08:34:04 [2290] - info: [build] Building language files 12/5 08:34:06 [2290] - info: [build] js => Completed in 9.108s 12/5 08:34:09 [2290] - info: [build] lang => Completed in 4.836s 12/5 08:34:09 [2290] - info: [build] Linking sound files 12/5 08:34:09 [2290] - info: [build] sound => Completed in 0.004s 12/5 08:34:09 [2290] - info: [build] Asset compilation successful. Completed in 12.235s. OK NodeBB Upgrade Complete!
This one:
[2017/2/27] New sorted set posts:votes...
took a looong time.Wouldn't it make more sense for you to do that before the production server was migrated? Like, for testing or something.
-
@wharrgarbl You must have missed when Ben was posting about the testing he was doing before the rollout
-
@wharrgarbl said in Why are we not rolling back?:
Wouldn't it make more sense for you to do that before the production server was migrated? Like, for testing or something.
@ben_lubar did that, but apparently didn't do much testing.
-
@izzion said in Why are we not rolling back?:
Pardon me for the mass-mention, but #trust_level_3 - if you have infiniscroll still enabled, can you please take 5 minutes to test these two replication cases and report back if you did or didn't get a page jump, what OS and browser you're using, and what your Settings > Pagination > Posts Per Page setting is?
When I had Posts per Page at 50, the "scroll down" case didn't cause problems, "scroll up" did.
After reading the other replies, I changed that setting to 20, and both cases caused problems.
I'm using Chrome58.0.3029.110 (64-bit)
on Windows 10.After some tinkering, I see two different problems here:
- The client sends a request "give me 20 posts (the page size) in this topic after post #19", but the server sends back a lot more than 20 (I got 45 = all the remaining posts in that topic). The client adds them all to the DOM and then removes the old posts keeping only the latest 40 posts, which removes the posts you were reading and a few more.
It seems to me that this pull request is the culprit – it adds acount
parameter to the request but looks forpostsPerPage
when handling it, and it doesn't de-hardcode infiniscroll's fixed page size of 40 posts. Whenever the list of visible posts changes, it only keeps that many posts in the DOM. The request fetches >40 posts, adds them to the page, then this trimmer removes all but the first/last 40 posts, and it all jellypotatoes apart. - When scrolling up, the client tries to keep your scroll position unchanged by measuring it in the old DOM, prepending the new posts, calculating the new target position, scrolling to it, and then removing the old posts. While it's scrolling, the browser raises the "hey, the window was scrolled" event, which the infinite scroll uses to decide if it needs to load more posts. Chrome (and probably other browsers these days) does its own bookkeeping for scroll position, which sometimes tricks this behaviour into thinking it needs to load more posts again, doubling the jellypotato.
- The client sends a request "give me 20 posts (the page size) in this topic after post #19", but the server sends back a lot more than 20 (I got 45 = all the remaining posts in that topic). The client adds them all to the DOM and then removes the old posts keeping only the latest 40 posts, which removes the posts you were reading and a few more.
-
you all laughed at me when I started using upvotes to track my position in a thread. Whose laughing now?!?!?!
Also, yes.
Also, why is it triggering the disappear-the-posts thing while loading the next set of posts? I think if it didn't do that we'd probably be fine. Who was responsible for that change?
-
@Tsaukpaetra said in Why are we not rolling back?:
you all laughed at me when I started using upvotes to track my position in a thread
I was annoyed by the notifications, but I'm ok since I completely disabled upvote notifications.
You also reinforced my desire that upvotes were limited like in Discourse.
-
@wharrgarbl said in Why are we not rolling back?:
@Tsaukpaetra said in Why are we not rolling back?:
you all laughed at me when I started using upvotes to track my position in a thread
I was annoyed by the notifications, but I'm ok since I completely disabled upvote notifications.
You also reinforced my desire that upvotes were limited like in Discourse.
You know you can disable upvote notifications... Right?
-
@wharrgarbl said in Why are we not rolling back?:
I completely disabled upvote notifications.
@Tsaukpaetra said in Why are we not rolling back?:
You know you can disable upvote notifications.
-
@DCoder Yeah, that would explain what happens to me in the vote balance topic (I scroll up to post 55, then every post vanishes, I'm sent to post 47, but all the posts after 47 are just gone).
-
@DCoder said in Why are we not rolling back?:
After some tinkering, I see two different problems here:
Yep...I commented out the call to
removeExtra
and the problem navigation goes away.
-
@DCoder
Hm, apparently default Chrome Dev Tools don't give me insight into the WebSockets requests.@DCoder, Do you have a login at community.nodebb.org that you can use to check this thread for the same behavior? Or can you let me know what you're doing to see the post data that's coming back on an infiniscroll event so I can repeat the test there?
Edit: Oh duh.
-
@wharrgarbl said:
I was annoyed by the notifications, but I'm ok since I completely disabled upvote notifications.
@Tsaukpaetra said:
You know you can disable upvote notifications... Right?
I think so.
-
-
@TDWTF-NodeBB-Development
Can someone help me understand what the configuration values in line 43 of src/socket.io/topics/infinitescroll.js are referencing? Per @DCoder's discovery and additional information in our test thread on comm.nodebb, I think there's a logic problem that's causing infiniscroll to send back the server's "max posts per page" rather than the client's "posts per page" setting and that's what's blowing things up, but I don't know what the config values are, so as I'm looking at the logic there it looks correct.
-
@izzion said in Why are we not rolling back?:
@TDWTF-NodeBB-Development
Can someone help me understand what the configuration values in line 43 of src/socket.io/topics/infinitescroll.js are referencing? Per @DCoder's discovery and additional information in our test thread on comm.nodebb, I think there's a logic problem that's causing infiniscroll to send back the server's "max posts per page" rather than the client's "posts per page" setting and that's what's blowing things up, but I don't know what the config values are, so as I'm looking at the logic there it looks correct.possible.... have you tried setting your posts per page setting in your settings configuration thingie to 10 (the prior value) and seeif the issue disappears?
-
@izzion
Specifically, the socket is emitting:Request for more posts:
42165["topics.loadMore", {tid: 10675, after: 70, count: 5, direction: 1, topicPostSort: "oldest_to_newest"}]
Response from server:
42165[..."posts":{array with max_postsPerPage values}]
OH!
The issue is that the socket is sending the request with a variable name "count", but the code is expecting a variable name "postsPerPage" (data.postsPerPage) -- which means that the Math.min that is comparing the socket request to the server's maxPostsPerPage setting is comparing a null against the server setting and always returns the server's setting.
-
Can we please try monkey patching line 43 of src/socket.io/topics/infinitescroll.js:
- var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.postsPerPage, 10) || meta.config.postsPerPage) - 1); + var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.count, 10) || meta.config.postsPerPage) - 1);
-
-
@izzion That's one part of it. For people who have page size configured to 50 (like me), an additional fix is needed here, where the client assumes it got < 40 new posts before removing the other posts. Boomzilla commented that line out, but that's not a proper fix ;)
-
@izzion said in Why are we not rolling back?:
Can we please try monkey patching line 43 of src/socket.io/topics/infinitescroll.js:
- var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.postsPerPage, 10) || meta.config.postsPerPage) - 1); + var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.count, 10) || meta.config.postsPerPage) - 1);
You know
diff
is a recognized HLJS type?- var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.postsPerPage, 10) || meta.config.postsPerPage) - 1); + var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.count, 10) || meta.config.postsPerPage) - 1);
-
@izzion said in Why are we not rolling back?:
Can we please try monkey patching line 43 of src/socket.io/topics/infinitescroll.js:
- var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.postsPerPage, 10) || meta.config.postsPerPage) - 1); + var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.count, 10) || meta.config.postsPerPage) - 1);
That seems to have done it, locally.
-
@boomzilla
Yaaay. Though that change wouldn't have an impact on the replication case you found with the Who is Using NodeBB thread, since your "count" setting is 20 and that's their server's postsPerPage setting there as well.But at least if we get back to where we're only having problems at the same frequency as Core, maybe we can isolate the other cases in a reproducible way.
-
@anotherusername
You presuppose that I bother with HLJS at all, rather than just slapping three backticks in and letting auto-detect do its thing.
-
@boomzilla said in Why are we not rolling back?:
@ben_lubar did that, but apparently didn't do much testing.
Hey, give him a break.
Ben did a lot of test, it's just that every time the jelly-potato-scrolling happened, he believed the problem was his crappy connection
-
@Tsaukpaetra said in Why are we not rolling back?:
you all laughed at me when I started using upvotes to track my position in a thread.
I thought you really liked my posts
-
@DCoder
Issue has been re-classified as a bug and they're intending to fix by changing the hard coded 40 to (postsPerPage x 2), plus the fix so that the client postsPerPage is honored as long as it's less than the server's max postsPerPage. Hopefully we'll have something to fix it fairly soon.Much thanks to everyone who helped verify the replication case and gather additional information to help us isolate the root cause!
-
@izzion The commit's in: we just need @ben_lubar to deploy it
-
@RaceProUK said in Why are we not rolling back?:
@izzion The commit's in: we just need @ben_lubar to
deployslowly upload itFTFY
-
@izzion CLOSED_WORKING_AS_WONT_FIX_UR_DOIN_IT_WRONG_USE_PAGINATION
-
@Arantor
been using pagination since.. well since forever because @node 's worthless streaming of posts doesn't work for shit.
-
@darkmatter I didn't but now I do and I'm better for it.
Toxic hellstew forums FTW.
-
@izzion said in Why are we not rolling back?:
Can we please try monkey patching line 43 of src/socket.io/topics/infinitescroll.js:
- var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.postsPerPage, 10) || meta.config.postsPerPage) - 1); + var infScrollPostsPerPage = Math.max(0, Math.min(meta.config.postsPerPage, parseInt(data.count, 10) || meta.config.postsPerPage) - 1);
Here's how our monkey patching thing works:
- Make a PR to NodeBB/NodeBB
- add
.diff
to the end of the URL - Copy the URL it redirects you to into Dockerfile in the pull requests section following the format of the other monkey patches.
-
@ben_lubar
Ah, well, then the patch is here: https://github.com/NodeBB/NodeBB/commit/70adcd64bcdd477ec91da93048e485fae62f5e31
-
@bb36e said in Why are we not rolling back?:
@Tsaukpaetra said in Why are we not rolling back?:
you all laughed at me when I started using upvotes to track my position in a thread.
I thought you really liked my posts
I do. But I like them because I've seen them. Whereas others use scripts to lessen the meaning meaninglessly.
-
Yay for languages where you can use variables and properties that don't exist without it being caught by the compiler!
-
@masonwheeler
Probably the one case where SQL NULL math would have been helpful (it would have kicked NULLs all the way back up the chain).
-
@izzion why stop at that? Surely things like, I dunno, relationships would be useful too...
-
@izzion said in Why are we not rolling back?:
Probably the one case where
SQL NULLIEEE NaN math would have been helpfulFTFY.
-
@Arantor said in Why are we not rolling back?:
@izzion why stop at that? Surely things like, I dunno, relationships would be useful too...
Next you'll be saying a database schema could help. Gosh.
-
@ben_lubar it's always been my friend...
-
@ben_lubar i don't always use a database schema, but when i do, i make it a star.
-
@Arantor
Well, I was more thinking specifically of the difference between how JS handled a Min comparison between NULL and a number, versus how SQL would have.But yes, also that.
-
@izzion how about using a saner language like PHP whose type jugging rules while insane because type juggling is insane, are less broken than JavaScript's?
Toxic hellstew!
-
@izzion said in Why are we not rolling back?:
Well, I was more thinking specifically of the difference between how JS handled a Min comparison between NULL and a number, versus how SQL would have.
? The expr was
Math.min(meta.config.postsPerPage, parseInt(data.count, 10) || meta.config.postsPerPage)
.
That'sMath.min(50, (undefined || 50))
, which intentionally replaces null with a number beforemin
ever sees it.ECMAScript in fact specifies the behaviour you like:
min ( [ value1 [ , value2 [ , … ] ] ] )
Given zero or more arguments, calls ToNumber on each of the arguments and returns the smallest of the resulting values.
...
If any value is NaN, the result is NaN.
...Yes, I read the language standards for fun sometimes.
-
@DCoder
Ah. I blame javascript for my inability to read javascript code.
-
@izzion said in Why are we not rolling back?:
@DCoder
Ah. I blame javascript for my inability to read javascript code.Where's your sense of adventure
-
@julianlam I left that in the closet along with my toxic hellstew forum. I can't speak for anyone else though.
-
@Arantor type juggling is the best kind of juggling.
and titty jiggling is the best kind of jiggling.
-
@darkmatter said in Why are we not rolling back?:
@Arantor type juggling is the best kind of juggling.
and titty jiggling is the best kind of jiggling.How are the jiggle physics coming now?