server,serverutils: simplify interfaces #109591
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knz
requested review from
michae2,
ericharmeling,
miretskiy,
herkolategan and
smg260
and removed request for
a team
|
This change is |
knz
removed request for
a team,
michae2,
ericharmeling,
miretskiy and
herkolategan
knz
force-pushed
the
20230828-test-interfaces2
branch
from
d848682 to
2fcb0c5
Compare
stevendanna
reviewed
Aug 30, 2023
knz
force-pushed
the
20230828-test-interfaces2
branch
from
2fcb0c5 to
738dea3
Compare
This is just poor phrasing of the commit message. Corrected. The actual preventing happens in #109612 where the interfaces are simply removed. |
This commit simplifies as follows: - it removes use of .SystemLayer() and .ApplicationLayer() from within the testServer code. This is ahead of a smarter implementation of these accessors in a later commit. - it renames "Stopper" to "AppStopper" in "ApplicationLayerInterface", to disambiguate it from the stopper at the top level testServer. Release note: None
knz
force-pushed
the
20230828-test-interfaces2
branch
from
738dea3 to
d9726e1
Compare
craig bot
pushed a commit
that referenced
this pull request
Sep 1, 2023
109464: server: batch large span stats requests r=zachlite a=zachlite Previously, span stats requests could only request a maximum of 500 spans at a time, otherwise the request would return an error. This commit adds transparent batching at both the system tenant's handler and the secondary tenant's handler. The cluster setting `server.span_stats.span_batch_limit` is used to control the size of the batches. The default value has been raised to 1000. Resolves #105638 Epic: CRDB-30635 Release note (ops change): Requests for database details or table details from the UI, or usages of `SHOW RANGES WITH DETAILS` are no longer subject to errors if the number of requested spans is too large. 109612: serverutils: implement the testserver interfaces indirectly r=stevendanna a=knz First commit from #109591. Epic: CRDB-18499 Fixes #107058. TLDR: unit tests using TestServer/TestCluster now produce warning and notice messages upon certain suspicious API usage. For example: ``` $ dev test //pkg/server/storage_api -f TestStatusGetFiles === RUN TestStatusGetFiles ... pkg/server/storage_api/storage_api_test_test/pkg/server/storage_api/files_test.go:46: (TestStatusGetFiles) NOTICE: .GetStatusClient() called via implicit interface ApplicationLayerInterface; HINT: consider using .ApplicationLayer().GetStatusClient() instead. conditional_wrap.go:193: TIP: consider replacing the test server initialization from: ts, db, kvDB := serverutils.StartServer(t, ...) // or: tc := serverutils.StartCluster(t, ...) ts := tc.Server(0) To: srv, db, kvDB := serverutils.StartServer(t, ...) defer srv.Stop(...) ts := srv.ApplicationLayer() // or: tc := serverutils.StartCluster(t, ...) ts := tc.Server(0).ApplicationLayer() ``` The following warnings/notices are implemented. **ApplicationLayerInterface** The problem here is that the implicit implementation of `ApplicationLayerInterface` inside `TestServerInterface` is likely misused. If/when the test server automatically starts a secondary tenant, the *implicit* `ApplicationLayerInterface` refers to the system tenant, whereas only `.ApplicationLayer()` refers to the secondary tenant. It's likely that the test code should use the latter. For this, a warning is printed when a method from the *implicit* `ApplicationLayerInterface` is called. The warning is not printed if the test has indicated (via `DefaultTestTenant: base.TestIsForStorageLayerAndNeedsASystemTenant`) that it is ever only interested in the storage layer. **StorageLayerInterface** We want to promote the use of the explicit interface accessor `.StorageLayer()` to access the methods of `StorageLayerInterface`. For this, a notice is printed when a method from the *implicit* `StorageLayerInterface` is called. The notice is not printed if the test has indicated (via `DefaultTestTenant: base.TestIsForStorageLayerAndNeedsASystemTenant`) that it is ever only interested in the storage layer. **TenantControlInterface** We want to promote the use of the explicit interface accessor `.TenantController()` to access the methods of `TenantControlInterface`. For this, a notice is printed when a method from the *implicit* `TenantControlInterface` is called. 109863: roachtest: add npgsql tests to ignorelist; not blocklist r=rafiss a=rafiss The previous commit 1928be2 accidentally put them in the wrong list. The blocklist is for tests that are expected to always fail, but a flaky test might pass sometimes. fixes #109806 fixes #109738 Release note: None 109872: sql: coherent handling of hidden session vars in info_schema r=rafiss a=knz Fixes #109870. Epic: CRDB-28893 Release note (bug fix): Certain SQL session variables are meant to be hidden from introspection but were showing up in `information_schema.session_variables`, which was incoherent with the handling in `pg_catalog.pg_settings`. This bug has now been fixed. Co-authored-by: Zach Lite <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit simplifies as follows:
it removes use of .SystemLayer() and .ApplicationLayer() from within the testServer code. This is ahead of a smarter implementation of these accessors in a later commit.
it renames "Stopper" to "AppStopper" in "ApplicationLayerInterface", to disambiguate it from the stopper at the top level testServer.
Release note: None
Fixes #109565.
Epic: CRDB-18499