A little bit of code cleanup and commenting

Author: jeremydw

src/server.ts

@@ -3,7 +3,6 @@ import {GoogleAuth} from 'google-auth-library';
 import * as fsPath from 'path';
 import express = require('express');
 import httpProxy = require('http-proxy');
-import {Manifest} from './manifest';
 
 const URL = 'https://storage.googleapis.com';
 const datastore = new Datastore();
@@ -12,9 +11,6 @@ const auth = new GoogleAuth({
 });
 
 const server = httpProxy.createProxyServer();
-interface ManifestCache {
-  [requestSha: string]: string;
-}
 
 const getManifest = async (siteId: string, branchOrRef: string) => {
   const keys = [
@@ -70,7 +66,6 @@ const parseHostname = (hostname: string) => {
 };
 
 export function createApp(siteId: string, branchOrRef: string) {
-  // const startupManifest = await getManifest(branchOrRef);
   console.log(`Starting server for site: ${siteId} @ ${branchOrRef}`);
 
   const app = express();
@@ -141,7 +136,8 @@ export function createApp(siteId: string, branchOrRef: string) {
       delete proxyRes.headers['x-guploader-uploadid'];
       // This cannot be "private, max-age=0" as this kills perf.
       // This also can't be a very small value, as it kills perf. 0036 seems to work correctly.
-      proxyRes.headers['cache-control'] = 'public, max-age=0036'; // The padded 0036 keeps the content length the same per upload.ts.
+      // The padded "0036" keeps the Content-Length identical with `3600`.
+      proxyRes.headers['cache-control'] = 'public, max-age=0036';
       proxyRes.headers['x-fileset-blob'] = blobKey;
       proxyRes.headers['x-fileset-ref'] = manifest.ref;
       proxyRes.headers['x-fileset-site'] = siteId;

src/upload.ts

@@ -1,16 +1,13 @@
-import * as cliProgress from 'cli-progress';
-import {asyncify, mapLimit} from 'async';
-import {Manifest, ManifestFile} from './manifest';
 import _colors = require('colors');
-
-import {Storage} from '@google-cloud/storage';
+import {asyncify, mapLimit} from 'async';
 import {Datastore} from '@google-cloud/datastore';
 import {entity} from '@google-cloud/datastore/build/src/entity';
+import {Manifest, ManifestFile} from './manifest';
+import {Storage} from '@google-cloud/storage';
+import * as cliProgress from 'cli-progress';
 
 const datastore = new Datastore();
-
 const DEFAULT_BUCKET = `${process.env.GCLOUD_PROJECT}.appspot.com`;
-
 const NUM_CONCURRENT_UPLOADS = 64;
 
 function getBlobPath(siteId: string, hash: string) {
@@ -68,62 +65,67 @@ export async function uploadManifest(
   const filesToUpload = force
     ? manifest.files
     : await findUploadedFiles(manifest, storageBucket);
-  const numFiles = filesToUpload.length;
+  const numTotalFiles = filesToUpload.length;
   console.log(
     `Found new ${filesToUpload.length} files out of ${manifest.files.length} total...`
   );
 
-  if (numFiles > 0) {
-    let totalTransferred = 0;
-    let numProcessed = 0;
+  if (numTotalFiles <= 0) {
+    finalize(manifest, ttl);
+  } else {
+    let bytesTransferred = 0;
+    let numProcessedFiles = 0;
     const startTime = Math.floor(Date.now() / 1000);
-    bar.start(numFiles, numProcessed, {
+    bar.start(numTotalFiles, numProcessedFiles, {
       speed: 0,
     });
+    // @ts-ignore
+    bar.on('stop', () => {
+      finalize(manifest, ttl);
+    });
 
     mapLimit(
       filesToUpload,
       NUM_CONCURRENT_UPLOADS,
       asyncify(async (manifestFile: ManifestFile) => {
-        const remotePath = getBlobPath(manifest.site, manifestFile.hash);
-
-        // NOTE: This was causing stale responses, even when rewritten by the client-server: 'public, max-age=31536000',
+        // NOTE: After testing, it seems we need a public Cache-Control with a
+        // minimum max age of ~3600 seconds (1 hour) for the "high performance"
+        // mode to kick in. Our best guess is that when this Cache-Control
+        // setting is used, responses are cached internally within GCP yielding
+        // much higher performance. That said, we don't want end users to
+        // receive potentially stale content, so the proxy response rewrites
+        // this header to 36 seconds (from 3600).
+        // The following docs indicate that Cache-Control doesn't apply for
+        // private blobs, however we've observed differences in performance per
+        // the above.
         // https://cloud.google.com/storage/docs/gsutil/addlhelp/WorkingWithObjectMetadata#cache-control
-        // NOTE: In order for GCS to respond extremely fast, it requires a longer cache expiry time.
-        // TODO: See if we can remove this from the proxy response without killing perf.
         const metadata = {
           cacheControl: 'public, max-age=3600',
           contentType: manifestFile.mimetype,
           metadata: {
             path: manifestFile.cleanPath,
           },
         };
-
         // TODO: Veryify this correctly handles errors and retry attempts.
+        const remotePath = getBlobPath(manifest.site, manifestFile.hash);
         const resp = await storageBucket.upload(manifestFile.path, {
           // NOTE: `gzip: true` must *not* be set here. Doing so interferes
           // with the proxied GCS response. Despite not setting `gzip: true`,
           // the response remains gzipped from the proxy server.
           destination: remotePath,
           metadata: metadata,
         });
-        totalTransferred += parseInt(resp[1].size);
+        bytesTransferred += parseInt(resp[1].size);
         const elapsed = Math.floor(Date.now() / 1000) - startTime;
-        bar.update((numProcessed += 1), {
-          speed: (totalTransferred / elapsed / (1024 * 1024)).toFixed(2),
+        const speed = (bytesTransferred / elapsed / (1024 * 1024)).toFixed(2);
+        bar.update((numProcessedFiles += 1), {
+          speed: speed,
         });
-        if (numProcessed === numFiles) {
+        if (numProcessedFiles === numTotalFiles) {
           bar.stop();
         }
       })
     );
-
-    // @ts-ignore
-    bar.on('stop', () => {
-      finalize(manifest, ttl);
-    });
-  } else {
-    finalize(manifest, ttl);
   }
 }
 
@@ -140,7 +142,7 @@ async function finalize(manifest: Manifest, ttl?: Date) {
   const manifestPaths = manifest.toJSON();
   const now = new Date();
 
-  // Create shortSha mapping for staging.
+  // Create shortSha mapping, so a shortSha can be used to lookup filesets.
   const key = datastore.key([
     'Fileset2Manifest',
     `${manifest.site}:ref:${manifest.shortSha}`,
@@ -152,7 +154,8 @@ async function finalize(manifest: Manifest, ttl?: Date) {
     paths: manifestPaths,
   });
 
-  // Create branch mapping for staging.
+  // Create branch mapping, so a branch name can be used to lookup filesets.
+  // TODO: Use clean branch name to avoid non-URL-safe branch names.
   if (manifest.branch) {
     const branchKey = datastore.key([
       'Fileset2Manifest',
@@ -172,10 +175,10 @@ async function finalize(manifest: Manifest, ttl?: Date) {
   // const routerKey = datastore.key(['Fileset2Router', manifest.site]);
   // const router =  await datastore.get(routerKey);
   // const entity = router && router[0];
-
   console.log(
     `Finalized upload for site: ${manifest.site} -> ${manifest.branch} @ ${manifest.shortSha}`
   );
+  // TODO: Allow customizing the staging URL using `fileset.yaml` configuration.
   console.log(
     `Staged: https://${manifest.site}-${manifest.shortSha}-dot-fileset2-dot-${process.env.GCLOUD_PROJECT}.appspot.com`
   );