Skip to content

Conversation

@ekuvardin
Copy link
Contributor

Connected to #335

Checkstyle
I couldn't find any checkstyle in projects and use default idea checkstyle.
Please give a link if you have one.

Solution

Try to make codestyle like in go ydb-platform/ydb-go-sdk#1519
ExecuteQuerySettings finally looks like

 ExecuteQuerySettings settings = ExecuteQuerySettings.newBuilder()
                        .withExecMode(QueryExecMode.EXECUTE)
                        .withResourcePool(TEST_RESOURCE_POOL_WITH_DELETE)
                        .withStatsMode(QueryStatsMode.FULL)
                        .build();

QuerySession doesn't changed and looks like this


 ExecuteQuerySettings settings = ....

                        session.createQuery("SELECT id, name FROM " + TEST_TABLE + " ORDER BY id;", TxMode.SERIALIZABLE_RW, Params.empty(), settings).execute().join();

Doubts
Take a look at TableClientImpl method executeDataQueryInternal code

        @Override
        public CompletableFuture<Result<DataQueryResult>> executeDataQueryInternal(
                String query, YdbTable.TransactionControl tx, Params prms, ExecuteDataQuerySettings settings) {
            YdbQuery.TransactionControl tc = mapTxControl(tx);
            ExecuteQuerySettings qs = ExecuteQuerySettings.newBuilder()
                    .withTraceId(settings.getTraceId())
                    .withRequestTimeout(settings.getTimeoutDuration())
                    .build();

            final AtomicReference<String> txRef = new AtomicReference<>("");
            QueryStream stream = querySession.new StreamImpl(querySession.createGrpcStream(query, tc, prms, qs)) {

In this code ExecuteQuerySettings don't use withResourcePool at all.
May be add withResourcePool in ExecuteDataQuerySettings?

Test
Also write some integration test.
All tests connected to Resource Pool I carried out in another class QueryIntegrationResourcePoolTest
For me it's better have different class for some features than have one big class with all test connected to Query.
Each feature needs some additional objects and checks specified for the feature but not the whole tests.

Some test problems
Problem with feature
In "ydbplatform/local-ydb:latest" you have to enable in tech.ydb.test.integration.YdbEnvironment
dockerFeatures = createParam("YDB_DOCKER_FEATURE_FLAGS", "enable_resource_pools"); to run test with pool
More detail I write in QueryIntegrationResourcePoolTest

I divide the test into 2 groups.
The first one passes, ignoring enable_resource_pools.
The second one runs only with enable_resource_pools -> I marked it as ignored. This annotation may be removed after the resource pool is enabled and removed from the experimental feature.

Test depending on feature
Some test give results depending on the enabled/disabled feature enable_resource_pools.
Take a look at QueryIntegrationResourcePoolTest.selectShouldFailWithUnknownResourcePollTest

When enable_resource_pools = true
select with unknown pool failed

When enable_resource_pools = false
select with unknown pool doesn't failed

I marked it as Ignore and will be enabled after enable_resource_pools = true

Some thoughts
Maybe just wait when the resource pool is removed from the experimental feature and becomes available in "ydbplatform/local-ydb:latest." after that merged in master. Then we can easily remove the annotation Ignore. Your repository, your call.

Ydb Image tested
24.3.11.13
24.4
24.4.4
24.5.1

Java tested
Java openjdk 21
Java openjdk 11

@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.83%. Comparing base (c831264) to head (1ce80ac).
Report is 12 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #382      +/-   ##
============================================
+ Coverage     58.75%   58.83%   +0.08%     
- Complexity     2167     2170       +3     
============================================
  Files           320      320              
  Lines         12894    12910      +16     
  Branches       1265     1266       +1     
============================================
+ Hits           7576     7596      +20     
+ Misses         4707     4704       -3     
+ Partials        611      610       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alex268 alex268 merged commit 88678c6 into ydb-platform:master Apr 4, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants