Merge #23816 #24616

23816: sql: stop using txn.OrigTimestamp to reset TableCollection r=vivekmenezes a=vivekmenezes Release note: None #23718 24616: ui: upgrade React 15.4 => 16.3 r=vilterp a=vilterp Fixes #18795 Release note: None Co-authored-by: Vivek Menezes <[email protected]> Co-authored-by: Pete Vilter <[email protected]>
cockroachdb · Apr 16, 2018 · f78ce2e · f78ce2e
3 parents 75d44eb + 04c07a4 + 9f0889a
commit f78ce2e
Show file tree

Hide file tree

Showing 23 changed files with 426 additions and 324 deletions.
diff --git a/pkg/sql/lease_test.go b/pkg/sql/lease_test.go
@@ -760,6 +760,91 @@ SELECT EXISTS(SELECT * FROM t.foo);
 	}
 }
 
+// TestDescriptorRefreshOnRetry tests that all descriptors acquired by
+// a query are properly released before the query is retried.
+func TestDescriptorRefreshOnRetry(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	params, _ := tests.CreateTestServerParams()
+
+	fooAcquiredCount := int32(0)
+	fooReleaseCount := int32(0)
+
+	params.Knobs = base.TestingKnobs{
+		SQLLeaseManager: &sql.LeaseManagerTestingKnobs{
+			LeaseStoreTestingKnobs: sql.LeaseStoreTestingKnobs{
+				// Set this so we observe a release event from the cache
+				// when the API releases the descriptor.
+				RemoveOnceDereferenced: true,
+				LeaseAcquiredEvent: func(table sqlbase.TableDescriptor, _ error) {
+					if table.Name == "foo" {
+						atomic.AddInt32(&fooAcquiredCount, 1)
+					}
+				},
+				LeaseReleasedEvent: func(table sqlbase.TableDescriptor, _ error) {
+					if table.Name == "foo" {
+						atomic.AddInt32(&fooReleaseCount, 1)
+					}
+				},
+			},
+		},
+	}
+	s, sqlDB, _ := serverutils.StartServer(t, params)
+	defer s.Stopper().Stop(context.TODO())
+
+	if _, err := sqlDB.Exec(`
+CREATE DATABASE t;
+CREATE TABLE t.foo (v INT);
+`); err != nil {
+		t.Fatal(err)
+	}
+
+	if atomic.LoadInt32(&fooAcquiredCount) > 0 {
+		t.Fatalf("CREATE TABLE has acquired a descriptor")
+	}
+
+	tx, err := sqlDB.Begin()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if _, err := tx.Exec(`
+		SELECT * FROM t.foo;
+		`); err != nil {
+		t.Fatal(err)
+	}
+
+	// Descriptor has been acquired.
+	if atomic.LoadInt32(&fooAcquiredCount) != 1 {
+		t.Fatal("descriptor not acquired")
+	}
+
+	// Descriptor has not been released.
+	if atomic.LoadInt32(&fooReleaseCount) > 0 {
+		t.Fatal("released descriptor")
+	}
+
+	if _, err := tx.Exec(
+		"SELECT CRDB_INTERNAL.FORCE_RETRY('1s':::INTERVAL)"); !testutils.IsError(
+		err, `forced by crdb_internal\.force_retry\(\)`) {
+		t.Fatal(err)
+	}
+
+	if atomic.LoadInt32(&fooAcquiredCount) > 1 {
+		t.Fatal("descriptor reacquired")
+	}
+
+	testutils.SucceedsSoon(t, func() error {
+		if atomic.LoadInt32(&fooReleaseCount) == 0 {
+			return errors.Errorf("didnt release descriptor")
+		}
+		return nil
+	})
+
+	if err := tx.Rollback(); err != nil {
+		t.Fatal(err)
+	}
+}
+
 // Test that a transaction created way in the past will use the correct
 // table descriptor and will thus obey the modififcation time of the
 // table descriptor.

diff --git a/pkg/sql/logictest/testdata/logic_test/prepare b/pkg/sql/logictest/testdata/logic_test/prepare
@@ -416,3 +416,19 @@ PREPARE x21701c AS SELECT * FROM kv WHERE k IS NOT DISTINCT FROM $1
 query II
 EXECUTE x21701c(NULL)
 ----
+
+# Test that a PREPARE statement after a CREATE TABLE in the same TRANSACTION
+# doesn't hang.
+subtest 24578
+
+statement ok
+BEGIN TRANSACTION
+
+statement ok
+create table bar (id integer)
+
+statement ok
+PREPARE forbar AS insert into bar (id) VALUES (1)
+
+statement ok
+COMMIT TRANSACTION
diff --git a/pkg/sql/table.go b/pkg/sql/table.go
@@ -27,7 +27,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
-	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 )
 
@@ -135,10 +134,6 @@ type uncommittedDatabase struct {
 // end of each transaction on the session, or on hitting conditions such
 // as errors, or retries that result in transaction timestamp changes.
 type TableCollection struct {
-	// The timestamp used to pick tables. The timestamp falls within the
-	// validity window of every table in leasedTables.
-	timestamp hlc.Timestamp
-
 	// leaseMgr manages acquiring and releasing per-table leases.
 	leaseMgr *LeaseManager
 	// A collection of table descriptor valid for the timestamp.
@@ -186,17 +181,6 @@ type dbCacheSubscriber interface {
 	waitForCacheState(cond func(*databaseCache) (bool, error)) error
 }
 
-// Check if the timestamp used so far to pick tables has changed because
-// of a transaction retry.
-func (tc *TableCollection) resetForTxnRetry(ctx context.Context, txn *client.Txn) {
-	if tc.timestamp != (hlc.Timestamp{}) &&
-		tc.timestamp != txn.OrigTimestamp() {
-		if err := tc.releaseTables(ctx, dontBlockForDBCacheUpdate); err != nil {
-			log.Warningf(ctx, "error releasing tables")
-		}
-	}
-}
-
 // getTableVersion returns a table descriptor with a version suitable for
 // the transaction: table.ModificationTime <= txn.Timestamp < expirationTime.
 // The table must be released by calling tc.releaseTables().
@@ -259,10 +243,6 @@ func (tc *TableCollection) getTableVersion(
 		}
 	}
 
-	// If the txn has been pushed the table collection is released and
-	// txn deadline is reset.
-	tc.resetForTxnRetry(ctx, flags.txn)
-
 	if refuseFurtherLookup, table, err := tc.getUncommittedTable(
 		dbID, tn, flags.required); refuseFurtherLookup || err != nil {
 		return nil, nil, err
@@ -299,7 +279,7 @@ func (tc *TableCollection) getTableVersion(
 		// know how to deal with, so propagate the error.
 		return nil, nil, err
 	}
-	tc.timestamp = origTimestamp
+
 	tc.leasedTables = append(tc.leasedTables, table)
 	log.VEventf(ctx, 2, "added table '%s' to table collection", tn)
 
@@ -328,10 +308,6 @@ func (tc *TableCollection) getTableVersionByID(
 		return table, nil
 	}
 
-	// If the txn has been pushed the table collection is released and
-	// txn deadline is reset.
-	tc.resetForTxnRetry(ctx, txn)
-
 	for _, table := range tc.uncommittedTables {
 		if table.ID == tableID {
 			log.VEventf(ctx, 2, "found uncommitted table %d", tableID)
@@ -364,7 +340,7 @@ func (tc *TableCollection) getTableVersionByID(
 		}
 		return nil, err
 	}
-	tc.timestamp = origTimestamp
+
 	tc.leasedTables = append(tc.leasedTables, table)
 	log.VEventf(ctx, 2, "added table '%s' to table collection", table.Name)
 
@@ -403,7 +379,6 @@ func (tc *TableCollection) releaseLeases(ctx context.Context) {
 
 // releaseTables releases all tables currently held by the TableCollection.
 func (tc *TableCollection) releaseTables(ctx context.Context, opt releaseOpt) error {
-	tc.timestamp = hlc.Timestamp{}
 	if len(tc.leasedTables) > 0 {
 		log.VEventf(ctx, 2, "releasing %d tables", len(tc.leasedTables))
 		for _, table := range tc.leasedTables {
@@ -550,16 +525,11 @@ func (tc *TableCollection) getUncommittedTable(
 func (tc *TableCollection) getAllDescriptors(
 	ctx context.Context, txn *client.Txn,
 ) ([]sqlbase.DescriptorProto, error) {
-	// If the txn has been pushed the table collection is released and txn
-	// deadline is reset.
-	tc.resetForTxnRetry(ctx, txn)
-
 	if tc.allDescriptors == nil {
 		descs, err := GetAllDescriptors(ctx, txn)
 		if err != nil {
 			return nil, err
 		}
-		tc.timestamp = txn.OrigTimestamp()
 		tc.allDescriptors = descs
 	}
 	return tc.allDescriptors, nil

diff --git a/pkg/ui/package.json b/pkg/ui/package.json
@@ -7,27 +7,24 @@
   "dependencies": {
     "analytics-node": "^3.0.0",
     "classnames": "^2.2.5",
-    "codemirror": "^5.25.2",
     "combokeys": "^2.4.6",
     "highlight.js": "^9.10.0",
     "lodash": "^4.17.4",
     "moment": "^2.18.1",
     "nvd3": "^1.8.5",
     "raw-loader": "^0.5.1",
     "rc-progress": "^2.2.1",
-    "react": "^15.4.2",
+    "react": "^16.3.1",
     "react-addons-create-fragment": "^15.4.2",
-    "react-codemirror": "^1.0.0",
     "react-copy-to-clipboard": "^5.0.1",
-    "react-dom": "^15.4.2",
-    "react-helmet": "^5.2.0",
     "react-motion": "^0.5.2",
+    "react-dom": "^16.3.1",
+    "react-helmet": "^5.2.0",
     "react-paginate": "^4.4.3",
     "react-redux": "^5.0.3",
-    "react-router": "<4.0.0",
+    "react-router": "^3.2.1",
     "react-router-redux": "<5.0.0",
-    "react-select": "^1.0.0-rc.4",
-    "react-sticky": "^5.0.5",
+    "react-select": "^1.2.1",
     "redux": "^3.6.0",
     "redux-saga": "^0.15.6",
     "redux-thunk": "^2.2.0",
@@ -46,25 +43,23 @@
     "@types/d3-drag": "^1.1.0",
     "@types/d3-timer": "^1.0.5",
     "@types/dagre-layout": "^0.8.0",
-    "@types/enzyme": "^2.8.12",
+    "@types/enzyme": "^3.1.9",
+    "@types/enzyme-adapter-react-16": "^1.0.2",
     "@types/fetch-mock": "^5.8.1",
     "@types/highlight.js": "^9.1.9",
     "@types/lodash": "^4.14.56",
     "@types/mocha": "^2.2.40",
     "@types/nvd3": "^1.8.36",
     "@types/prop-types": "^15.5.1",
-    "@types/react": "^15.0.39",
-    "@types/react-codemirror": "^0.2.6",
+    "@types/react": "^16.3.6",
     "@types/react-copy-to-clipboard": "^4.2.5",
-    "@types/react-dom": "^0.14.23",
+    "@types/react-dom": "^16.0.5",
     "@types/react-helmet": "^5.0.5",
-    "@types/react-motion": "^0.0.23",
     "@types/react-paginate": "^4.3.1",
     "@types/react-redux": "^4.4.38",
     "@types/react-router": "<4.0.0",
     "@types/react-router-redux": "<5.0.0",
-    "@types/react-select": "^1.0.45",
-    "@types/react-sticky": "^5.0.1",
+    "@types/react-select": "^1.2.6",
     "@types/redux-thunk": "^2.1.0",
     "@types/sinon": "2.1.0",
     "babel-core": "^6.24.0",
@@ -77,7 +72,8 @@
     "css-loader": "^0.28.0",
     "d3": "<4.0.0",
     "dagre-layout": "^0.8.0",
-    "enzyme": "^2.9.1",
+    "enzyme": "^3.3.0",
+    "enzyme-adapter-react-16": "^1.1.1",
     "express": "^4.15.2",
     "fetch-mock": "^5.9.4",
     "file-loader": "^0.11.1",
@@ -99,8 +95,6 @@
     "nib": "^1.1.2",
     "prop-types": "^15.5.10",
     "protobufjs": "^6.7.3",
-    "react-addons-test-utils": "^15.4.2",
-    "react-test-renderer": "^15.5.4",
     "rimraf": "^2.6.1",
     "sinon": "2.1.0",
     "source-map-loader": "^0.2.0",
@@ -119,8 +113,5 @@
   "engines": {
     "node": ">=6",
     "yarn": ">=1.0.0"
-  },
-  "resolutions": {
-    "@types/react": "~15.0.39"
   }
 }
diff --git a/pkg/ui/src/enzymeInit.ts b/pkg/ui/src/enzymeInit.ts
@@ -0,0 +1,6 @@
+import Enzyme from "enzyme";
+import Adapter from "enzyme-adapter-react-16";
+
+// As of v3, Enzyme requires an "adapter" to be initialized.
+// See https://github.com/airbnb/enzyme/blob/master/docs/guides/migration-from-2-to-3.md
+Enzyme.configure({ adapter: new Adapter() });
diff --git a/pkg/ui/src/index.tsx b/pkg/ui/src/index.tsx
@@ -1,7 +1,5 @@
 /// <reference path="../node_modules/protobufjs/stub-node.d.ts" />
 
-import "codemirror/lib/codemirror.css";
-import "codemirror/theme/neat.css";
 import "nvd3/build/nv.d3.min.css";
 import "react-select/dist/react-select.css";
 import "styl/app.styl";

diff --git a/pkg/ui/src/views/app/containers/layout/index.tsx b/pkg/ui/src/views/app/containers/layout/index.tsx
@@ -1,7 +1,6 @@
 import React from "react";
 import { Helmet } from "react-helmet";
 import { RouterState } from "react-router";
-import { StickyContainer } from "react-sticky";
 
 import NavigationBar from "src/views/app/components/layoutSidebar";
 import TimeWindowManager from "src/views/app/containers/timewindow";
@@ -15,14 +14,16 @@ import AlertBanner from "src/views/app/containers/alertBanner";
  */
 export default class extends React.Component<RouterState, {}> {
   render() {
-    return <div>
-      <Helmet titleTemplate="%s | Cockroach Console" defaultTitle="Cockroach Console" />
-      <TimeWindowManager/>
-      <AlertBanner/>
-      <NavigationBar/>
-      <StickyContainer className="page">
-        { this.props.children }
-      </StickyContainer>
-    </div>;
+    return (
+      <div>
+        <Helmet titleTemplate="%s | Cockroach Console" defaultTitle="Cockroach Console" />
+        <TimeWindowManager/>
+        <AlertBanner/>
+        <NavigationBar/>
+        <div className="page">
+          { this.props.children }
+        </div>
+      </div>
+    );
   }
 }
diff --git a/pkg/ui/src/views/app/containers/timewindow/timewindow.spec.tsx b/pkg/ui/src/views/app/containers/timewindow/timewindow.spec.tsx
@@ -5,6 +5,7 @@ import * as sinon from "sinon";
 import moment from "moment";
 import _ from "lodash";
 
+import "src/enzymeInit";
 import { TimeWindowManagerUnconnected as TimeWindowManager } from "./";
 import * as timewindow from "src/redux/timewindow";
 

diff --git a/pkg/ui/src/views/cluster/containers/clusterOverview/cluster.styl b/pkg/ui/src/views/cluster/containers/clusterOverview/cluster.styl
@@ -13,10 +13,6 @@
 .database-summary-title h2
   padding-top 0
 
-.page-config
-  padding-top 0
-  padding-bottom 0
-
 .cluster-overview
   .cluster-summary
     background-color white

diff --git a/pkg/ui/src/views/cluster/containers/events/events.spec.tsx b/pkg/ui/src/views/cluster/containers/events/events.spec.tsx
@@ -5,6 +5,7 @@ import _ from "lodash";
 import Long from "long";
 import * as sinon from "sinon";
 
+import "src/enzymeInit";
 import * as protos from  "src/js/protos";
 import { EventBoxUnconnected as EventBox, EventRow, getEventInfo } from "src/views/cluster/containers/events";
 import { refreshEvents } from "src/redux/apiReducers";

diff --git a/pkg/ui/src/views/cluster/containers/events/index.tsx b/pkg/ui/src/views/cluster/containers/events/index.tsx
@@ -34,13 +34,7 @@ export interface SimplifiedEvent {
   content: React.ReactNode;
 }
 
-// Specialization of generic SortedTable component:
-//   https://github.com/Microsoft/TypeScript/issues/3960
-//
-// The variable name must start with a capital letter or JSX will not recognize
-// it as a component.
-// tslint:disable-next-line:variable-name
-const EventSortedTable = SortedTable as new () => SortedTable<SimplifiedEvent>;
+class EventSortedTable extends SortedTable<SimplifiedEvent> {}
 
 export interface EventRowProps {
   event: Event$Properties;