Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@
import java.util.Map;
import java.util.Properties;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Executor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;

/**
* An implementation of {@link java.sql.Connection} for establishing a connection with BigQuery and
Expand All @@ -74,7 +74,6 @@ public class BigQueryConnection extends BigQueryNoOpsConnection {
private final BigQueryJdbcCustomLogger LOG = new BigQueryJdbcCustomLogger(this.toString());
String connectionClassName = this.toString();
private final String connectionId;
private static final AtomicLong connectionIdCounter = new AtomicLong(1);
private static final String DEFAULT_JDBC_TOKEN_VALUE = "Google-BigQuery-JDBC-Driver";
private static final String DEFAULT_VERSION = "0.0.0";
private HeaderProvider headerProvider;
Expand Down Expand Up @@ -149,7 +148,7 @@ public class BigQueryConnection extends BigQueryNoOpsConnection {
}

BigQueryConnection(String url, DataSource ds) throws IOException {
this.connectionId = String.valueOf(connectionIdCounter.getAndIncrement());
this.connectionId = UUID.randomUUID().toString();
try (BigQueryJdbcMdc.MdcCloseable mdc =
BigQueryJdbcMdc.registerInstance(this, this.connectionId)) {
LOG.finest("++enter++");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,15 @@

package com.google.cloud.bigquery.jdbc;

import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicLong;

/**
* Lightweight MDC implementation for the BigQuery JDBC driver using InheritableThreadLocal.
* Allocates a dedicated, independent InheritableThreadLocal object per concrete BigQueryConnection
* instance.
*/
class BigQueryJdbcMdc {
Comment thread
keshavdandeva marked this conversation as resolved.
private static final AtomicLong nextId = new AtomicLong(1);
private static final ConcurrentHashMap<BigQueryConnection, InheritableThreadLocal<String>>
instanceLocals = new ConcurrentHashMap<>();
private static final ConcurrentHashMap<BigQueryConnection, String> instanceIds =
Expand All @@ -41,9 +40,8 @@ static MdcCloseable registerInstance(BigQueryConnection connection, String id) {
instanceIds.computeIfAbsent(
connection,
k -> {
String suffix =
(id != null && !id.isEmpty()) ? id : String.valueOf(nextId.getAndIncrement());
return "JdbcConnection-" + suffix;
String baseId = (id != null && !id.isEmpty()) ? id : UUID.randomUUID().toString();
return "BQ-JDBC-" + baseId;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: don't think we need BQ-JDBC- or any other prefix.. They're internal and only relevant to stitch together telemetry/logs. Can be just a uuid

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user application many have logs generated from other sources. For the file name I think it is useful if we keep the BQ-JDBC prefix. Same with the connection ID, if the user Application is logging from multiple sources , it will be helpful to identify the logs generated by the JDBC Driver.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that's the internal ID, not the filename

});

currentConnectionId.set(cleanId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
import java.util.concurrent.ConcurrentHashMap;
import java.util.logging.FileHandler;
import java.util.logging.Handler;
Expand Down Expand Up @@ -53,7 +55,13 @@ class PerConnectionFileHandler extends Handler {
}

private String getLogFilePath(String id) {
return baseLogPath.resolve("BigQuery-" + id + ".log").toString();
String uuid = id;
if (id.startsWith("BQ-JDBC-")) {
uuid = id.substring("BQ-JDBC-".length());
}
String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMddHHmmss"));
String shortUuid = uuid.length() >= 4 ? uuid.substring(0, 4) : uuid;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for that check, uuid will never be shorter than 4

return baseLogPath.resolve("BQ-JDBC-" + timestamp + "-" + shortUuid + ".log").toString();
}

private FileHandler createFileHandler(String id) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,18 @@ public void tearDown() {
@Test
public void testRegisterAndRetrieveConnectionId() {
BigQueryJdbcMdc.registerInstance(mockConnection1, "123");
assertEquals("JdbcConnection-123", BigQueryJdbcMdc.getConnectionId());
assertEquals("BQ-JDBC-123", BigQueryJdbcMdc.getConnectionId());
}

@Test
public void testRemoveInstance() {
BigQueryJdbcMdc.registerInstance(mockConnection1, "1");
assertEquals("JdbcConnection-1", BigQueryJdbcMdc.getConnectionId());
assertEquals("BQ-JDBC-1", BigQueryJdbcMdc.getConnectionId());

BigQueryJdbcMdc.removeInstance(mockConnection1);
// Note: removeInstance does not clear currentConnectionId on the current thread
// based on current implementation.
assertEquals("JdbcConnection-1", BigQueryJdbcMdc.getConnectionId());
assertEquals("BQ-JDBC-1", BigQueryJdbcMdc.getConnectionId());

BigQueryJdbcMdc.clear();
assertNull(BigQueryJdbcMdc.getConnectionId());
Expand All @@ -67,7 +67,7 @@ public void testRemoveInstance() {
@Test
public void testClearContext() {
BigQueryJdbcMdc.registerInstance(mockConnection1, "456");
assertEquals("JdbcConnection-456", BigQueryJdbcMdc.getConnectionId());
assertEquals("BQ-JDBC-456", BigQueryJdbcMdc.getConnectionId());

BigQueryJdbcMdc.clear();
assertNull(BigQueryJdbcMdc.getConnectionId());
Expand All @@ -76,7 +76,7 @@ public void testClearContext() {
@Test
public void testThreadInheritance() throws InterruptedException {
BigQueryJdbcMdc.registerInstance(mockConnection1, "parent");
assertEquals("JdbcConnection-parent", BigQueryJdbcMdc.getConnectionId());
assertEquals("BQ-JDBC-parent", BigQueryJdbcMdc.getConnectionId());

AtomicReference<String> childConnectionId = new AtomicReference<>();
CountDownLatch latch = new CountDownLatch(1);
Expand All @@ -90,7 +90,7 @@ public void testThreadInheritance() throws InterruptedException {
childThread.start();
assertTrue(latch.await(5, TimeUnit.SECONDS));

assertEquals("JdbcConnection-parent", childConnectionId.get());
assertEquals("BQ-JDBC-parent", childConnectionId.get());
}

@Test
Expand Down Expand Up @@ -144,17 +144,17 @@ public void testThreadIsolation() throws InterruptedException {

assertTrue(testFinished.await(5, TimeUnit.SECONDS));

assertEquals("JdbcConnection-A", threadAIdBeforeB.get());
assertEquals("BQ-JDBC-A", threadAIdBeforeB.get());
assertNull(threadBIdBeforeRegister.get());
assertEquals("JdbcConnection-B", threadBIdAfterRegister.get());
assertEquals("JdbcConnection-A", threadAIdAfterB.get());
assertEquals("BQ-JDBC-B", threadBIdAfterRegister.get());
assertEquals("BQ-JDBC-A", threadAIdAfterB.get());
}

@Test
public void testMdcCloseableClearsContext() {
try (BigQueryJdbcMdc.MdcCloseable mdc =
BigQueryJdbcMdc.registerInstance(mockConnection1, "789")) {
assertEquals("JdbcConnection-789", BigQueryJdbcMdc.getConnectionId());
assertEquals("BQ-JDBC-789", BigQueryJdbcMdc.getConnectionId());
}
assertNull(BigQueryJdbcMdc.getConnectionId());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.LogRecord;
import org.junit.jupiter.api.AfterEach;
Expand Down Expand Up @@ -51,10 +52,18 @@ public void tearDown() {
BigQueryJdbcMdc.clear();
}

private Optional<Path> findLogFile(String suffix) throws IOException {
return Files.list(tempDir)
.filter(
p ->
p.getFileName().toString().startsWith("BQ-JDBC-")
&& p.getFileName().toString().endsWith(suffix))
.findFirst();
}

@Test
public void testInitialization() {
Path defaultLog = tempDir.resolve("BigQuery-Jdbc-default.log");
assertTrue(Files.exists(defaultLog));
public void testInitialization() throws IOException {
assertTrue(findLogFile("-Jdbc.log").isPresent());
}

@Test
Expand All @@ -63,8 +72,9 @@ public void testPublishDefault() throws IOException {
handler.publish(record);
handler.flush();

Path defaultLog = tempDir.resolve("BigQuery-Jdbc-default.log");
String content = new String(Files.readAllBytes(defaultLog));
Optional<Path> defaultLog = findLogFile("-Jdbc.log");
assertTrue(defaultLog.isPresent());
String content = new String(Files.readAllBytes(defaultLog.get()));
assertTrue(content.contains("Test message default"));
}

Expand All @@ -76,24 +86,24 @@ public void testPublishConnectionSpecific() throws IOException {
handler.publish(record);
handler.flush();

Path connLog = tempDir.resolve("BigQuery-JdbcConnection-123.log");
assertTrue(Files.exists(connLog));
String content = new String(Files.readAllBytes(connLog));
Optional<Path> connLog = findLogFile("-123.log");
assertTrue(connLog.isPresent());
String content = new String(Files.readAllBytes(connLog.get()));
assertTrue(content.contains("Test message connection 123"));
}

@Test
public void testCloseHandler() {
public void testCloseHandler() throws IOException {
BigQueryJdbcMdc.registerInstance(mockConnection, "456");

LogRecord record = new LogRecord(Level.INFO, "Test message connection 456");
handler.publish(record);
handler.flush();

Path connLog = tempDir.resolve("BigQuery-JdbcConnection-456.log");
assertTrue(Files.exists(connLog));
Optional<Path> connLog = findLogFile("-456.log");
assertTrue(connLog.isPresent());

handler.closeHandler("JdbcConnection-456");
assertTrue(Files.exists(connLog));
handler.closeHandler("BQ-JDBC-456");
assertTrue(Files.exists(connLog.get()));
}
}
Loading