fix(gatsby): persist pages between runs #28590

pieh · 2020-12-13T10:13:04Z

Description

Approach in #28351 turned out to be unfeasble - we do need entire page object actually to preserve backward compatiblity for DELETE_PAGE action.

To handle potential OOM issues when persisting state, I expanded state sharding during persistance from just nodes to both nodes and now persisted pages. Right now it's copied & pasted sharding done for nodes - I could have DRY it (I still can if reviewers will request it :) ), but didn't think DRYing it is worth adding mental overhead for added abstractions - thoughts?

This PR build on work already done by @redabacha (in #28316) borrowing test change (unskip) already done (in #28351).

TODO:

garbage collect stateful pages during bootstrap (see comment)

Related Issues

fixes #28281
fixes #26520
[ch19791]

pieh · 2020-12-13T15:19:16Z

packages/gatsby/src/redux/__tests__/index.js

+    const nodeShardsScenarios = [
+      {
+        numberOfNodes: 50,
+        simulatedNodeObjectSize: 5 * 1024 * 1024,
+        expectedNumberOfNodeShards: 1,
+      },
+      {
+        numberOfNodes: 5,
+        simulatedNodeObjectSize: 0.6 * 1024 * 1024 * 1024,
+        expectedNumberOfNodeShards: 3,
+      },
+    ]
+    const pageShardsScenarios = [
+      {
+        numberOfPages: 50,
+        simulatedPageObjectSize: 10 * 1024 * 1024,
+        expectedNumberOfPageShards: 1,
+      },
+      {
+        numberOfPages: 5,
+        simulatedPageObjectSize: 0.9 * 1024 * 1024 * 1024,
+        expectedNumberOfPageShards: 5,
+      },
+    ]


This is to construct scenario matrix - and we check each combination. I didn't go over the board - primary purpose of this is that sharding for pages and nodes is separate and that one doesn't affect the other.

Something to think about is wether we want to add some edge cases here - like when number of nodes or pages is equal 0 - for now implementation for those is that we discard persisted state when either is 0 - it does make a whole lot of sense for nodes as core creates some nodes of their own even if user doesn't use any source plugins. It's a bit different for pages - gatsby doesn't create pages (well except for gatsby develop creating dev-404-page ...). I am not sure if it's worth handling this case more gracefully? Do we want to optimize for scenarios where there are 0 pages?

pieh · 2020-12-13T15:20:44Z

packages/gatsby/src/redux/persist.ts

+
+  const pages: Array<[string, IGatsbyPage]> = [].concat(...pagesChunks)
+
+  if (!pagesChunks.length) {


Potentially problematic - this will discard state if there are 0 pages - do we want to optimize for those cases? (I don't even know if gatsby actually "works" if there are 0 pages TBH)

When way to go about this is to add pagesCount to redux.rest part and compare it I guess? Cache will get invalidated anyway after upgrade due to pagesChunk.length being 0 so I think cache shape "version" could be handled gracefully

If there were no pages then it's probably no big deal to bust the cache anyways.

As per off-thread discussion. I think you can merge this PR in a patch without this check, if you wanted to and then add the check in a minor bump later.

packages/gatsby/src/redux/persist.ts

pvdz · 2020-12-14T10:06:39Z

packages/gatsby/src/redux/persist.ts

+
+  const pages: Array<[string, IGatsbyPage]> = [].concat(...pagesChunks)
+
+  if (!pagesChunks.length) {


As per off-thread discussion. I think you can merge this PR in a patch without this check, if you wanted to and then add the check in a minor bump later.

pieh · 2020-12-20T09:29:03Z

Additional note on this - this needs additional handling done for createPagesStatefully: createPages does "garbage collect" and it works as-is with this code, but createPagesStatefully doesn't have garbage collection so we will never clean those pages up. This means that if user do gatsby build -> remove page from src/pages/ -> gatsby build, with this code as-is we will crash with (currently):

 ERROR #85913  GRAPHQL

There was a problem reading the file: /Users/misiek/dev/gatsby-starter-blog/src/pages/multiple-exports.js

File: src/pages/multiple-exports.js



  Error: ENOENT: no such file or directory, open '/Users/misiek/dev/gatsby-starter-blog/src/pages/multiple-exports.js'

failed extract queries from components - 0.324s

TODO item: garbage collect stateful pages in bootstrap

pieh · 2021-02-10T17:29:12Z

If merged, we should revert #29431

…for updatedAt and snapshot testing)

…s per test scenario

LekoArts

LGTM

LekoArts · 2021-04-21T14:20:17Z

packages/gatsby/src/redux/persist.ts

@@ -74,7 +89,7 @@ function guessSafeChunkSize(values: Array<[string, IGatsbyNode]>): number {
  }

  // Sends a warning once if any of the chunkSizes exceeds approx 500kb limit
-  if (maxSize > 500000) {
+  if (showMaxSizeWarning && maxSize > 500000) {
    report.warn(
      `The size of at least one page context chunk exceeded 500kb, which could lead to degraded performance. Consider putting less data in the page context.`


We've talked about adding information about what page exceeds this limit, but that's for a follow-up (not in this PR)

* add stateful page to artifacts tests * fix(gatsby): garbage collect stateful pages * Revert "fix: clear tracked queries when deleting stale page-data files (#29431)" (#30848) This reverts commit 478cf68.

LekoArts

🌮

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 13, 2020

pieh mentioned this pull request Dec 13, 2020

fix(gatsby): invalidate queries if page context changes between runs #28351

Closed

1 task

pieh added topic: query invalidation* and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Dec 13, 2020

pieh force-pushed the fix/no-rebuild-bug branch from f6c1a86 to aa421fe Compare December 13, 2020 10:59

pieh commented Dec 13, 2020

View reviewed changes

pieh marked this pull request as ready for review December 14, 2020 09:04

pvdz reviewed Dec 14, 2020

View reviewed changes

pieh force-pushed the fix/no-rebuild-bug branch from 65f3f72 to eb6689b Compare December 20, 2020 09:30

pieh marked this pull request as draft December 20, 2020 20:35

pieh mentioned this pull request Dec 26, 2020

fix(gatsby): garbage collect stateful pages #28760

Merged

vladar mentioned this pull request Jan 19, 2021

New data in createPage does not update between builds #29086

Closed

pieh mentioned this pull request Jan 22, 2021

refactor(conditional-page-build): tracking PR #29140

Closed

pieh mentioned this pull request Feb 10, 2021

fix: clear tracked queries when deleting stale page-data files #29431

Merged

pieh closed this in #29431 Feb 10, 2021

pieh reopened this Feb 10, 2021

Reda Bacha and others added 8 commits April 13, 2021 11:07

fix(gatsby): add pages to saved redux state

8b0d1d8

chore: update test snapshots

4f3561a

unskip context change scenario for data tracking tests

69c5178

mock Date.now so persistance tests don't rely on actual time (needed …

fff1e1d

…for updatedAt and snapshot testing)

refactor persistance tests a bit to allow for creating different page…

3be8d73

…s per test scenario

shard pages state (on top of existing sharding for nodes)

b949625

drop page count check (there actually might be valid cases for 0 pages)

1e63476

only show page context size warning for pages (and not nodes)

619041c

pieh force-pushed the fix/no-rebuild-bug branch from eb6689b to 619041c Compare April 13, 2021 09:32

pieh added 2 commits April 13, 2021 12:15

fix lint

fc115fd

adjust snapshot

5a46fda

pieh mentioned this pull request Apr 13, 2021

Revert "fix: clear tracked queries when deleting stale page-data files (#29431) #30848

Merged

test(artifacts): add case for changing page context

135ddd6

pieh changed the title ~~fix(gatsby): invalidate queries if page context changes between runs~~ fix(gatsby): persist pages between runs Apr 13, 2021

pieh marked this pull request as ready for review April 14, 2021 11:33

LekoArts previously approved these changes Apr 21, 2021

View reviewed changes

fix(gatsby): garbage collect stateful pages (#28760)

7880240

* add stateful page to artifacts tests * fix(gatsby): garbage collect stateful pages * Revert "fix: clear tracked queries when deleting stale page-data files (#29431)" (#30848) This reverts commit 478cf68.

pieh dismissed LekoArts’s stale review via 7880240 April 21, 2021 19:49

LekoArts approved these changes Apr 22, 2021

View reviewed changes

pieh merged commit a0b31bc into master Apr 22, 2021

pieh deleted the fix/no-rebuild-bug branch April 22, 2021 08:48

pieh mentioned this pull request Oct 17, 2022

chore(gatsby): Remove LOCKED_IN feature flags #36810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gatsby): persist pages between runs #28590

fix(gatsby): persist pages between runs #28590

pieh commented Dec 13, 2020 •

edited

Loading

pieh Dec 13, 2020

pieh Dec 13, 2020

pieh Dec 14, 2020

pvdz Dec 14, 2020

pvdz Dec 14, 2020

pvdz Dec 14, 2020

pieh commented Dec 20, 2020

pieh commented Feb 10, 2021

LekoArts left a comment

LekoArts Apr 21, 2021

LekoArts left a comment


		const pages: Array<[string, IGatsbyPage]> = [].concat(...pagesChunks)

		if (!pagesChunks.length) {

fix(gatsby): persist pages between runs #28590

fix(gatsby): persist pages between runs #28590

Conversation

pieh commented Dec 13, 2020 • edited Loading

Description

Related Issues

pieh Dec 13, 2020

Choose a reason for hiding this comment

pieh Dec 13, 2020

Choose a reason for hiding this comment

pieh Dec 14, 2020

Choose a reason for hiding this comment

pvdz Dec 14, 2020

Choose a reason for hiding this comment

pvdz Dec 14, 2020

Choose a reason for hiding this comment

pvdz Dec 14, 2020

Choose a reason for hiding this comment

pieh commented Dec 20, 2020

pieh commented Feb 10, 2021

LekoArts left a comment

Choose a reason for hiding this comment

LekoArts Apr 21, 2021

Choose a reason for hiding this comment

LekoArts left a comment

Choose a reason for hiding this comment

pieh commented Dec 13, 2020 •

edited

Loading