Skip to content

Storage V2 - query node schema and mappings - #2515

Merged
shamil-gadelshin merged 24 commits into
Joystream:storage_v2from
Lezek123:storagev2-query-node
Sep 9, 2021
Merged

shamil-gadelshin merged 24 commits into
Joystream:storage_v2from
Lezek123:storagev2-query-node

Conversation

@Lezek123

@Lezek123 Lezek123 commented Jul 5, 2021 •

Copy link
Copy Markdown
Contributor

The PR contains:

  • Update to hydra-v3 (3.1.0-alpha.1)
  • Storage v2 mappings
    Currently only the ones that should be required for storage-node and distributor-node to work.
    Giza and Sumer mappings have been separated and only the Giza mappings are currently beeing built.
    Further work on updating Sumer mappings is required and will be part of another PR.
  • Protobuf metadata standards for Giza
    Currently two separate libraries exist: @joystream/metadata-protobuf (Giza) and @joystream/content-metadata-protobuf (Sumer) - they will be merged together in a separate PR.
  • Small @joystream/types adjustments taken from Distributor node #2582 (fixed DynamicBagCreationPolicy, added some utility types, renamed some distribution module types)

@Lezek123
Lezek123 force-pushed the storagev2-query-node branch from cf02ca4 to 829d2a1 Compare September 6, 2021 12:47
@Lezek123
Lezek123 marked this pull request as ready for review September 6, 2021 17:11
@Lezek123 Lezek123 mentioned this pull request Sep 7, 2021

@shamil-gadelshin shamil-gadelshin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. All current storage node queries could be modified to the working state.

There are several issues found:

  • unresolved TODOs in several files
  • the query-node/mappings project has failed checks (lints, formatting)
  • the metadata-protobuf project fails linting
  • Shouldn't we use the bagId format similar to the storage and distribution node (static:council) ?

Comment thread query-node/schemas/storage.graphql
Comment thread query-node/mappings/giza/storage/index.ts
export async function storage_StorageOperatorMetadataSet({ event, store }: EventContext & StoreContext): Promise<void> {
const [bucketId, , metadataBytes] = new Storage.StorageOperatorMetadataSetEvent(event).params
const storageBucket = await getStorageBucketWithOperatorMetadata(store, bucketId.toString())
storageBucket.operatorMetadata = await processStorageOperatorMetadata(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why don't we throw an error here on invalid metadata similar to other invalid data?

@Lezek123 Lezek123 Sep 9, 2021 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The errors are only thrown on some unexpected state which should never occur if the node is working correctly.
Invalid metadata is a different case, because the runtime does not check its validity, so it's still a normal condition that the metadata is invalid. We're just logging this and performing no action in this case.

Throwing an error here would basically shut down the query node processor.

Comment thread query-node/mappings/giza/storage/index.ts Outdated
Comment thread query-node/schemas/storage.graphql Outdated
@Lezek123

Lezek123 commented Sep 9, 2021 •

Copy link
Copy Markdown
Contributor Author

the query-node/mappings project has failed checks (lints, formatting)

The main reason for this is that on master the autogenerated query-node files are comitted, so the query node can be built without connecting to a running node and fetching the metadata. It also allows us to format those files to be inline with the prettier config, linter rules etc. and make some other custom changes in them.

Comitting autogenerated files has a few disadvantages though, which can be observed for @joystream/types (as one example) - it makes the repo substantially larger, the PRs much harder to parse and it generates lots of conflicts when merging different branches. I think on master we also had some manual changes in the query-node autogenerated files, which would need to be either automated or manually repeated each time the the files are re-generated.

There are mutiple ways we can address this:

  1. keep comitting autogenerated files the way it was done on master
  2. make running a dockerized node a prerequisite for most/all of the checks in order to fetch the chain metadata for generation scripts
  3. commit current chain metadata as json into the repo (it's ~500 KB currenty)

I would vote for option 3 as a final solution, as having chain metadata in the repo could be useful in many different ways and updating it seems quite trivial.

I think that, especially during development, option number1 could be quite inconvenient.

I'll try to address this issue in my next PR, since in this one the sumer/master mappings have not even been updated yet.

@Lezek123

Lezek123 commented Sep 9, 2021

Copy link
Copy Markdown
Contributor Author

the metadata-protobuf project fails linting

I couldn't reproduce this issue, I'm only getting some warnings:

$ yarn workspace @joystream/metadata-protobuf checks
yarn workspace v1.22.4
yarn run v1.22.4
$ tsc --noEmit --pretty && prettier ./ --check && yarn lint
Checking formatting...
All matched files use Prettier code style!
$ eslint ./src --ext .ts

/home/leszek/projects/joystream/joystream-ws-1/metadata-protobuf/src/types.ts
  13:34  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  14:37  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

/home/leszek/projects/joystream/joystream-ws-1/metadata-protobuf/src/utils.ts
  8:54  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

✖ 3 problems (0 errors, 3 warnings)

Done in 11.07s.
Done in 11.55s.

@shamil-gadelshin

Copy link
Copy Markdown
Contributor

I couldn't reproduce this issue, I'm only getting some warnings:

I meant the warnings. Sorry for the confusion.

@shamil-gadelshin

Copy link
Copy Markdown
Contributor

There are mutiple ways we can address this:

If we don't want to commit the generated code shouldn't we just add the whole generated folder to the linter's ignore list as a simple solution?

@Lezek123

Lezek123 commented Sep 9, 2021

Copy link
Copy Markdown
Contributor Author

I meant the warnings. Sorry for the confusion.

Fixed (924da13)

If we don't want to commit the generated code shouldn't we just add the whole generated folder to the linter's ignore list as a simple solution?

Oh, I didn't realize linter will work on the project that does not build.
I added generated files to prettierignore and eslintignore and modified the query-node workflow - it now only runs linter and prettier checks, but does not attempt to actually build the node (97e3196)

I think this should work as a temporary solution before we decide on the metadata.

@shamil-gadelshin shamil-gadelshin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@shamil-gadelshin
shamil-gadelshin merged commit 7d36e17 into Joystream:storage_v2 Sep 9, 2021
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.

2 participants