Skip to content

[WIP] ✨ added db import polling endpoints#10340

Closed
vikaspotluri123 wants to merge 3 commits intoTryGhost:masterfrom
vikaspotluri123:feat/import-api-polling
Closed

[WIP] ✨ added db import polling endpoints#10340
vikaspotluri123 wants to merge 3 commits intoTryGhost:masterfrom
vikaspotluri123:feat/import-api-polling

Conversation

@vikaspotluri123
Copy link
Copy Markdown
Member

refs #9388 (this is just the backend)

  • Add GET /ghost/api/v2/admin/db/import
    • returns the import state (currently importing or not) as well as the result of the last import

schema:

{
  db: [],
  importing: Boolean,
  lastResult: {
    id: Number,
    success: Boolean,
    errors: undefined (success) | Array (!success)
    problems: Array (success) | undefined (!success)
  }
}
  • Add POST /ghost/api/v2/admin/db/import
    • takes a file to import
    • returns a randomly generated id which will be included in the lastResult field of the GET endpoint

Copy link
Copy Markdown
Member Author

@vikaspotluri123 vikaspotluri123 left a comment

Choose a reason for hiding this comment

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

Functional tests will come after an initial review 😄

Comment thread core/server/api/v2/db.js Outdated
Comment thread core/server/web/api/v2/admin/routes.js Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since this is APIv2, it might be possible to alter the POST .../db method once changes are made to the admin client

refs TryGhost#9388

- GET /ghost/api/v2/admin/db/import returns the import state
 (currently importing or not) as well as the result of the last import

 schema:
 {
   db: [],
   importing: Boolean,
   lastResult: {
     id: Number,
     success: Boolean,
     errors: undefined (success) | Array (!success)
     problems: Array (success) | undefined (!success)
   }
 }

- POST /ghost/api/v2/admin/db/import
  - receives a file to import
  - Returns a randomly generated id which will be included in the `lastResult`
  field of the GET endpoint once the import completes
Comment thread core/server/lib/abstract-poller.js Outdated
@vikaspotluri123
Copy link
Copy Markdown
Member Author

I'm not exactly sure why, but it looks like something is blocking 😬 The first import status request responds quickly, but the second request takes 7+ seconds. I don't have a large import to test, so I've been adding a 10s delay to after the import completes.

@kirrg001
Copy link
Copy Markdown
Contributor

Hm let me know if you need help, then i can help debugging

@vikaspotluri123
Copy link
Copy Markdown
Member Author

I'm not entirely sure where to start 🤔

Here's the network log:
image

First 2 import responses work properly (first is PUT and second is running: true), but the last one delays until the import finishes (but technically responds properly).

Running DEBUG=ghost:* node --prof index.js with no changes (relative to HEAD) shows a very clear order of model creation running, then the URL service, and finally webhook processing.

The import I'm using is a few repeats of the default posts which was exported as part of qa for this PR

@kirrg001
Copy link
Copy Markdown
Contributor

I don't see this behaviour.

INFO [2019-02-19 17:49:36] "DELETE /ghost/api/v2/admin/db/" 204 8595ms
INFO [2019-02-19 17:49:41] "POST /ghost/api/v2/admin/db/import/" 202 31ms
INFO [2019-02-19 17:49:41] "GET /ghost/api/v2/admin/db/import/" 200 9ms
INFO [2019-02-19 17:49:42] "GET /ghost/api/v2/admin/db/import/" 200 202ms
INFO [2019-02-19 17:49:43] "GET /ghost/api/v2/admin/db/import/" 200 23ms
INFO [2019-02-19 17:49:44] "GET /ghost/api/v2/admin/db/import/" 200 20ms
INFO [2019-02-19 17:49:45] "GET /ghost/api/v2/admin/db/import/" 200 17ms
INFO [2019-02-19 17:49:46] "GET /ghost/api/v2/admin/db/import/" 200 19ms
INFO [2019-02-19 17:49:47] "GET /ghost/api/v2/admin/db/import/" 200 23ms

But what I do see is: lot's of error messages from xmlrpc, which is weird, because we skip the service if options.importing is true 🤔

NAME: InternalServerError
CODE: ETIMEDOUT
MESSAGE: Request timed out

level:normal

"The xmlrpc service was unable to send a ping request, your blog will continue to function."
"If you get this error repeatedly, please seek help on https://docs.ghost.org."
InternalServerError: Request timed out

@kirrg001
Copy link
Copy Markdown
Contributor

But what I do see is: lot's of error messages from xmlrpc, which is weird, because we skip the service if options.importing is true 🤔

I guess this should be fixed via 2fd5964.

@kirrg001
Copy link
Copy Markdown
Contributor

@vikaspotluri123 Do you need any further feedback/help? :)

@vikaspotluri123
Copy link
Copy Markdown
Member Author

Not at the moment, but I'll let you know 😄

@vikaspotluri123
Copy link
Copy Markdown
Member Author

I don't see this behaviour.

Maybe it's a different node version? I'm testing with 10.15.3

INFO [2019-03-13 04:04:29] "POST /ghost/api/v2/admin/db/import/" 202 96ms 
INFO [2019-03-13 04:04:29] "GET /ghost/api/v2/admin/db/import/" 200 32ms 
INFO [2019-03-13 04:04:42] "GET /ghost/api/v2/admin/db/import/" 200 11715ms 
INFO [2019-03-13 04:04:42] "GET /ghost/api/v2/admin/settings/?type=blog%2Ctheme%2Cprivate%2Cmembers" 200 73ms 
INFO [2019-03-13 04:04:42] "GET /ghost/api/v2/admin/users/1/?include=roles" 200 61ms 
INFO [2019-03-13 04:04:42] "GET /favicon.ico?t=1552449882488" 200 6ms

FWIW I'm also not starting with a fresh instance, though I doubt that makes a difference wrt import times 🤔

@kirrg001
Copy link
Copy Markdown
Contributor

Hmm will test again on Monday.

@kirrg001
Copy link
Copy Markdown
Contributor

@vikaspotluri123 Sorry didn't have time to review again. Will do next Monday 🤺

@naz naz self-assigned this May 13, 2019
@naz naz self-requested a review May 13, 2019 17:37
@naz
Copy link
Copy Markdown
Contributor

naz commented May 13, 2019

Will have a look at these on Monday @vikaspotluri123 One thing that definitely needs more thinking is naming of the API endpoints as these are a bit unusual cases and should probably be treated differently to normal ones where they return resource(s) 🤔

@stale
Copy link
Copy Markdown

stale Bot commented Aug 11, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the stale [triage] Issues that were closed to to lack of traction label Aug 11, 2019
@naz naz removed the stale [triage] Issues that were closed to to lack of traction label Aug 12, 2019
@naz
Copy link
Copy Markdown
Contributor

naz commented Aug 12, 2019

Not stale, just needs a broader discussion about the approach and introduction of new endpoints within the team.

@naz
Copy link
Copy Markdown
Contributor

naz commented Aug 26, 2019

@vikaspotluri123 as a part of a more aggressive push to reduce the number of open issues that are not actively worked on the #9388 has been closed down. Closing this PR to keep "open PRs" tab clean. We definitely will come back to this once main issue is reopened/higher in priorities as the approach seems correct to me but needs more planning and broader team discussion as mentioned above 😉

@naz naz closed this Aug 26, 2019
@vikaspotluri123 vikaspotluri123 deleted the feat/import-api-polling branch November 4, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants