Uh oh!
There was an error while loading. Please reload this page.
[Vector clock] Clock work (2/3) - #772
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
* Migration to add trackBlockchainId to Files and remove extraneous UUID colums * lint * Start modifying routes to work with blockchainId for tracks * bug fixes * simplify nodesync create db props * Add read only mode * Lint fix * excellent test coverage * migration changes * More fixes * db only nodesync * code review fixes * bug fixes
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
SidSethi
left a comment
There was a problem hiding this comment.
bunch of comments, mostly small / TODOs
| module.exports = { | ||
| up: async (queryInterface, Sequelize) => { | ||
| // Scope of the migration is: |
| console.log('STARTING MIGRATION 20200911004845-allow-track-and-audiusUsers-appends') | ||
| await queryInterface.sequelize.query(` | ||
| BEGIN; |
There was a problem hiding this comment.
TODO improve readability
| ALTER TABLE "Files_new" DROP COLUMN "trackUUID"; | ||
| ALTER TABLE "Files" RENAME TO "Files_old"; | ||
| ALTER TABLE "Files_new" RENAME TO "Files"; | ||
| DROP TABLE "Files_old" CASCADE; |
There was a problem hiding this comment.
TODO document that dropping table will drop all constraints (as opposed to dropping constrained rows)
| CREATE INDEX "Files_trackBlockchainId_idx" ON public."Files" USING btree ("trackBlockchainId"); | ||
| -- add in the foreign key constraint from Files to other tables | ||
| -- No fkey from Files to Tracks because we don't have a unique constraint on trackUUID or blockchainId on Tracks so postgres would reject the fkey |
There was a problem hiding this comment.
instead of a full fkey, we can add a trigger to check existence. should not be too hard. does not need to go in with this release but a TODO / fast-follow would be good (i know how to do it and can handle later this week). without a join table i think this is a useful safety check that does not require much work
from: https://www.postgresql.org/message-id/20080916101610.Y70889%40megazone.bigpanda.com
| ALTER TABLE "Tracks" DROP CONSTRAINT "blockchainId_unique_idx"; | ||
| -- add a not null constraint to Tracks blockchainId, just run a delete query to remove any outstanding Tracks without a blockchainId in case there are any | ||
| DELETE FROM "Tracks" WHERE "blockchainId" IS NULL; |
There was a problem hiding this comment.
lets document that this is safe to do since the number of un-associated tracks is 12 and all of them are from 2019 before we had current simplified logic and therefore no chance of them being re-associated
| if (!req.session.nodeIsPrimary || !req.session.creatorNodeEndpoints || !Array.isArray(req.session.creatorNodeEndpoints)) return | ||
| const [primary, ...secondaries] = req.session.creatorNodeEndpoints | ||
| req.logger.error(`SIDTEST: primary ${primary} calling sync against: ${secondaries}`) |
| @@ -0,0 +1,36 @@ | |||
| const { sendResponse, errorResponseServerError } = require('../../apiHelpers') | |||
There was a problem hiding this comment.
TODO remove all of this?
| @@ -0,0 +1,52 @@ | |||
| 'use strict' | |||
| module.exports = (sequelize, DataTypes) => { | |||
| // TODO - why is this not externally accessible? | |||
There was a problem hiding this comment.
don't think this is a blocker for merge, but i want to leave this comment in since it is something we should resolve
| /** | ||
| * Returns latest clock value stored in CNodeUsers entry given wallet, or -1 if no entry found | ||
| */ | ||
| app.get('/users/clock_status/:walletPublicKey', handleResponse(async (req, res) => { |
There was a problem hiding this comment.
TODO - add TODO comment to modify to accept wallets array for easy cross-cnode querying
| const { handleResponse, successResponse, errorResponseServerError } = require('../apiHelpers') | ||
| const axios = require('axios') | ||
| module.exports = function (app) { |
There was a problem hiding this comment.
add TODO to remove this file + document it is temporary
| @@ -0,0 +1,43 @@ | |||
| /** | |||
There was a problem hiding this comment.
| // Retrieve all segment durations as map(segment srcFilePath => segment duration) | ||
| codeBlockTimeStart = Date.now() | ||
| const segmentDurations = await getSegmentsDuration(req.fileName, req.file.destination) |
There was a problem hiding this comment.
TODO function name is confusing, change to getSegmentDurations
| is_unlisted: true | ||
| }, | ||
| cnodeUserUUID: req.session.cnodeUserUUID | ||
| const tracks = (await models.sequelize.query( |
There was a problem hiding this comment.
TODO document this query
Uh oh!
There was an error while loading. Please reload this page.
Trello Card Link
https://trello.com/c/01XE53VH
Description - TODO
Services
Does it touch a critical flow like Discovery indexing, Creator Node track upload, Creator Node gateway, or Creator Node file system?
Delete an option.
How Has This Been Tested? - TODO
Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.
Include log analysis if applicable.
Please list the unit test(s) you added to verify your changes.