[PECOBLR-348] Added putFiles method in DBFSVolume client by shivam2680 · Pull Request #823 · databricks/databricks-jdbc

shivam2680 · 2025-05-12T11:15:10Z

Description

Added putFiles in Volume client to support async upload of files.
Design Doc

Testing

Driver Test
Integration Test
E2E test
Ingestion Team Benchmarks

Benchmark Results

Number of files	File Size (bytes)	Duration (ms)	Files/sec
100	8	4404	22
100	16	2164	46
100	160	2258	44
100	500	2504	39
1000	8	15961	62
1000	16	13116	76
1000	160	15285	65
1000	500	16396	60
4000	8	56740	70
4000	16	58010	68
4000	160	61217	65
4000	500	56372	70
10000	8	144293	69
10000	16	147235	67
10000	160	155257	64

Additional Notes to the Reviewer

* Fix UC volume GET operation * Add tests

…ed by workspace (#604) * init * fmt

* Added the GET and DELETE inside the DBFS client * Added java doc and refractored workspace client initialisation * Added unit tests * Improved the unit test coverage to 85% * Fixed the DBFS client to use ConnectionContext for initializing * Refractored the code for Volume Factory * Added java docs

* Adding changes for Fixing async execution * Add tests * Fixes as per comments * Add tests

* init * init * init * init

* Update SDK version * fix tests as apiClient is being used in sdk now

* Revert "revert (#595)" This reverts commit 56ef04f. * Fix uber minimal * fix version

* column size and nullable col fixed

…-and-deploy (#612)

* Telemetry * Add client * Add telemetry client * Minor fix * More changes * Changes for telemetry * Add more details in Telemetry impl * Add tests * Fix typo * Add more details * Updated error msg * Fix coverage

* parser * more complex data type logic * complete complex data types * more tests * refine * integration test * add test to driver test * removed secret * fmt * fix unsupported test * integration test working * fmt * comments * fmt * update method sig * fmt * comments * comments * comments * fmt * fmt * add * more extensive testing * more extensive testing * more extensive testing * more extensive testing * more extensive testing

* Send intial connection log to unauth telemetry endpoint * Add tests + remove debug logs * fmt * Add tests * Add tests * Add tests for classes i didn't add:) * add getters/setters in systemConfiguration + improve coverage

- Run execution queries in `async` mode - Run metadata queries in `async` mode - Refactor result retrieval in Thrift by improving response handling and fixing polling - Refactor credential refreshing by transitioning from Thrift accessor to Thrift transport - Competing driver, SQL Gateway, and SQL Execution API also execute the queries in async mode - Minor code cleanup Running queries in async mode and using polling will be beneficial: - Freeing up server resources under high load instead of holding the connection for ~5 seconds (direct results) - If query takes more than 5 seconds, server will anyway require polling from client - Didn't notice any regressions (slightly better performance) under high concurrent load when running queries in async mode. Some empirical results below: - Running a simple select query on a table with 10-15 columns for 10K rows in a concurrent environment (250 connections executing query in parallel) - Average results when running on `main` ``` Average query time across connections: 73873.58ms Min query time: 22075ms Max query time: 88997ms ``` - Average results when running in `async` mode ``` Average query time across connections: 68382.95ms Min query time: 24718ms Max query time: 95282ms ```

…ch (#622) This adds a workflow to run JDBC comparator in a separate branch for GA readiness. Note that this does not affect the main branch.

Add databricks protected runners

Use databricks runners

Setup PAT correctly

…y paths (#620) * change file paths * change file paths * change file paths * change file paths * change file paths * change file paths * change file paths

gopalldb · 2025-05-19T08:04:08Z

  }

+  @Override
+  public int getMaxConcurrentPresignedRequests() {


let's rename this to UCVolumeUploadBatchSize

It's not batch size. This is the number of parallel requests that driver sends for fetching pre-signed urls.

gopalldb · 2025-05-19T08:07:23Z

+    List<CompletableFuture<VolumePutResult>> futures = new ArrayList<>(objectPaths.size());
+
+    // Initialize lists with null. Later, set values at the correct index
+    for (int i = 0; i < objectPaths.size(); i++) {


Why is this step needed? To handle partial failures?

and maintain result order.

jayantsing-db

few comments inline. Could you please remove redundant fake service tests and stubs? i think fake-service does not provide the required fidelity as it is a client upload.

jayantsing-db · 2025-05-22T20:50:25Z

+    }
+
+    // Remove null entries from uploadRequests. Files that don't exist will be null
+    uploadRequests.removeIf(Objects::isNull);


futures seem to be a parallel array. no modifications needed to that array?

These are 2 different arrays,
futures is just track result order
uploadRequests is used to send parallel requests, if a file doesn't exist, we won't send upload request for that file.

jayantsing-db · 2025-05-22T21:24:27Z

+
+    this.source = source;
+    this.contentLength = contentLength;
+    this.contentType = Objects.requireNonNull(contentType, "contentType");


if this throws exception, are resources like inputstream closed?

Actually, this case won't arise as we are setting ContentType.APPLICATION_OCTET_STREAM always while constructing this producer. I can maybe remove this field itself.

okay thanks

jayantsing-db · 2025-05-22T21:26:28Z

+        channel.endStream();
+        return;
+      }
+      int read = source.read(buf, 0, (int) Math.min(buf.length, contentLength - totalSent));


this seems to be a blocking call for async thread pool. can we use nio channels if not too complex?

I feel based on the current upload latencies, current impl is fine. I agree this is blocking, but is limited by buf size (16KB). A pure NIO pipeline might get complicated

jayantsing-db · 2025-05-22T21:29:15Z

+        channel.endStream();
+        return;
+      }
+      int read = source.read(buf, 0, (int) Math.min(buf.length, contentLength - totalSent));


overflow risks with (int) Math.min(buf.length, contentLength - totalSent)?

Not really,
buf.length is always 16384(16KB), so Math.min(buf.length, contentLength - totalSent) has max value 16384 only.

jayantsing-db · 2025-05-22T21:29:54Z

+    }
+    try {
+      source.close();
+    } catch (IOException ignore) {


silent exception swallowing?

throwing ex now.

jayantsing-db · 2025-05-22T21:30:52Z

+        channel.endStream();
+      } else {
+        totalSent += read;
+        channel.write(ByteBuffer.wrap(buf, 0, read));


should we check if the channel can accept data?

introduced a currentChunk ByteBuffer. helps in identifying how many bytes channel accepts and retries until buffer is empty.

jayantsing-db · 2025-05-22T21:33:09Z

+        endOfStream = true;
+        channel.endStream();


should we check if we have actually sent expected number of bytes and fail accordingly?

gopalldb · 2025-05-23T05:21:29Z

  final WorkspaceClient workspaceClient;
  final ApiClient apiClient;
  private final String allowedVolumeIngestionPaths;
+  private static int MAX_CONCURRENT_PRESIGNED_REQUESTS = 50;


where is this default value used?

used to initialize the presignedUrlSemaphore that controls the rate limiting for presigned URL requests

gopalldb and others added 30 commits December 10, 2024 15:22

Fix UC volume GET operation (#602)

41e5cc4

* Fix UC volume GET operation * Add tests

Skip SEA compatibility check if null or empty DBSQL version is return…

a93c4b7

…ed by workspace (#604) * init * fmt

[PECO-2136] Changes to fix async execution (#603)

f46c957

* Adding changes for Fixing async execution * Add tests * Fixes as per comments * Add tests

Skips SEA check when dbsql version string is blank space (#605)

153cba6

* init * init * init * init

[PECO-1898] Update SDK version to resolve CVEs (#591)

9a0e918

* Update SDK version * fix tests as apiClient is being used in sdk now

Fix for async interface in SDK client (#606)

e8ceb7f

Cut a new release 0.9.8-oss (#607)

3d4dc3c

Deploy reduced POM during release (#608)

1c96614

* Revert "revert (#595)" This reverts commit 56ef04f. * Fix uber minimal * fix version

[PECO-2135] getColumns discrepancy with Databricks Driver (#609)

899f451

* column size and nullable col fixed

Fix for nexus staging (#611)

b473b7f

Nexus staging plugin's auto-release can't work with gpg plugin's sign…

22808ed

…-and-deploy (#612)

Add client code for Telemetry in JDBC (#600)

0a84eca

* Telemetry * Add client * Add telemetry client * Minor fix * More changes * Changes for telemetry * Add more details in Telemetry impl * Add tests * Fix typo * Add more details * Updated error msg * Fix coverage

Send intial connection log to unauth telemetry endpoint (#614)

d6c8f06

* Send intial connection log to unauth telemetry endpoint * Add tests + remove debug logs * fmt * Add tests * Add tests * Add tests for classes i didn't add:) * add getters/setters in systemConfiguration + improve coverage

Run JDBC comparator

2a5ab75

fmt

f2fefb1

Add workflow to run JDBC Driver Comparison in a separate private bran…

8c225c0

…ch (#622) This adds a workflow to run JDBC comparator in a separate branch for GA readiness. Note that this does not affect the main branch.

Create OSS connection first

f99e205

Remove PWD from OSS connection

add14b3

Update runJdbcComparator.yml

e81bf2e

Add databricks protected runners

Update runJdbcComparator.yml

e17cb94

Use databricks runners

Setup PAT correctly

5a98d17

Update runJdbcComparator.yml

c6454ce

Setup PAT correctly

Change integration test cached API call file paths to Windows-friendl…

852d69a

…y paths (#620) * change file paths * change file paths * change file paths * change file paths * change file paths * change file paths * change file paths

Add default to URL

0a888f6

Exclude telemetry

12a1f73

Verify PWD

ecc4c4f

Change env var name

fa376b2

update puObject stubs

209c82c

shivam2680 temporarily deployed to azure-prod May 16, 2025 08:53 — with GitHub Actions Inactive

added comments

5994ea9

shivam2680 temporarily deployed to azure-prod May 16, 2025 09:48 — with GitHub Actions Inactive

shivam2680 marked this pull request as ready for review May 19, 2025 04:46

shivam2680 requested review from deeksha-db, gopalldb, jayantsing-db, jprakash-db, madhav-db and samikshya-db May 19, 2025 04:47

gopalldb reviewed May 19, 2025

View reviewed changes

jayantsing-db requested changes May 22, 2025

View reviewed changes

gopalldb reviewed May 23, 2025

View reviewed changes

used java.nio.file

f2876af

shivam2680 temporarily deployed to azure-prod May 27, 2025 09:55 — with GitHub Actions Inactive

improved InputStreamFixedLenProducer

69ed761

shivam2680 temporarily deployed to azure-prod May 27, 2025 13:19 — with GitHub Actions Inactive

shivam2680 requested review from gopalldb and jayantsing-db and removed request for jayantsing-db May 27, 2025 13:27

jayantsing-db closed this Jun 20, 2025

jayantsing-db force-pushed the main branch from 33f02ce to cef0a00 Compare June 20, 2025 19:55

shivam2680 mentioned this pull request Jun 23, 2025

[PECOBLR-348] Added putFiles method in DBFSVolume client #860

Merged

Conversation

shivam2680 commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Benchmark Results

Additional Notes to the Reviewer

Uh oh!

gopalldb May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayantsing-db left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shivam2680 May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shivam2680 commented May 12, 2025 •

edited

Loading

gopalldb May 19, 2025 •

edited

Loading

shivam2680 May 27, 2025 •

edited

Loading