From 7a144b5ed5da1feae47873e26661e153a4a4d427 Mon Sep 17 00:00:00 2001
From: Won Jun Jang <wjang@uber.com>
Date: Thu, 5 Apr 2018 17:54:51 -0400
Subject: [PATCH] address comments

Signed-off-by: Won Jun Jang <wjang@uber.com>
---
 src/throttler/remote_throttler.js  | 23 +++++++++++------------
 test/throttler/remote_throttler.js |  7 +++++++
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/throttler/remote_throttler.js b/src/throttler/remote_throttler.js
index 15944e87a..ba281756c 100644
--- a/src/throttler/remote_throttler.js
+++ b/src/throttler/remote_throttler.js
@@ -59,9 +59,9 @@ export default class RemoteThrottler {
    * @param {object} [options.logger] - optional logger, see _flow/logger.js
    * @param {object} [options.metrics] - instance of Metrics object
    * @param {number} [options.refreshIntervalMs] - interval in milliseconds that determines how often credits
-   * are fetched (0 to not refresh, NOT RECOMMENDED!)
+   * are fetched
    * @param {number} [options.initialDelayMs] - interval in milliseconds that determines how soon after initialization
-   * credits are first fetched (0 to not refresh, NOT RECOMMENDED!).
+   * credits are first fetched
    * @param {string} [options.host] - host for jaeger-agent, defaults to 'localhost'
    * @param {number} [options.port] - port for jaeger-agent for /credits endpoint
    * @param {function} [options.onCreditsUpdate] - callback function once credits are updated. Used for testing.
@@ -74,7 +74,7 @@ export default class RemoteThrottler {
     this._host = options.host || DEFAULT_THROTTLER_HOST;
     this._port = options.port || DEFAULT_THROTTLER_PORT;
 
-    this._credits = Object.create(null);
+    this._credits = {};
     this._onCreditsUpdate = options.onCreditsUpdate;
 
     this._initialDelayTimeoutHandle = setTimeout(
@@ -97,9 +97,8 @@ export default class RemoteThrottler {
     if (operation in this._credits) {
       return this._isAllowed(operation);
     }
+    // Credits for the operation will be asynchronously fetched
     this._credits[operation] = 0;
-    // If seen for the first time, async fetch credits
-    this._refreshCredits();
     return false;
   }
 
@@ -118,7 +117,7 @@ export default class RemoteThrottler {
       return;
     }
     const keys = Object.keys(this._credits);
-    if (keys.length == 0) {
+    if (keys.length === 0) {
       // No point fetching credits if there's no operations to fetch
       return;
     }
@@ -132,15 +131,15 @@ export default class RemoteThrottler {
   }
 
   _fetchCredits(operations: any) {
-    let serviceName: string = encodeURIComponent(this._serviceName);
-    let uuid: string = encodeURIComponent(this._uuid);
-    let ops = operations.map(encodeURIComponent).join('&operation=');
-    let url = `/credits?service=${serviceName}&uuid=${uuid}&operation=${ops}`;
+    const serviceName: string = encodeURIComponent(this._serviceName);
+    const uuid: string = encodeURIComponent(this._uuid);
+    const ops: string = operations.map(encodeURIComponent).join('&operation=');
+    const url: string = `/credits?service=${serviceName}&uuid=${uuid}&operation=${ops}`;
 
-    let success: Function = body => {
+    const success: Function = body => {
       this._parseCreditResponse(body);
     };
-    let error: Function = err => {
+    const error: Function = err => {
       this._logger.error(`Error in fetching credits: ${err}.`);
       this._metrics.throttlerUpdateFailure.increment(1);
     };
diff --git a/test/throttler/remote_throttler.js b/test/throttler/remote_throttler.js
index 98777a9f4..1d07a3e82 100644
--- a/test/throttler/remote_throttler.js
+++ b/test/throttler/remote_throttler.js
@@ -70,6 +70,7 @@ describe('RemoteThrottler should', () => {
       done();
     };
     assert.isNotOk(throttler.isAllowed(operation));
+    throttler._refreshCredits();
   });
 
   it('log an error if _refreshCredits is called prior to UUID being set', () => {
@@ -77,6 +78,12 @@ describe('RemoteThrottler should', () => {
     assert.equal(logger._errorMsgs.length, 1);
   });
 
+  it('not fetch credits if uuid is invalid', () => {
+    throttler = new RemoteThrottler(serviceName);
+    throttler.setProcess({ uuid: null });
+    throttler._refreshCredits();
+  });
+
   it("return false for _isAllowed if operation isn't in _credits or operation has no credits", () => {
     assert.isNotOk(
       throttler._isAllowed(operation),