-
Notifications
You must be signed in to change notification settings - Fork 27
[PECOBLR-1408] [PECOBLR-1407] [PECOBLR-1409][PECOBLR-1411] Address telemetry code audit comments- part1 #1163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
tejassp-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Connection ref count never decrements to zero. Should be fixed.
| private InputStream inputStream; | ||
| private int chunkReadyTimeoutSeconds = | ||
| Integer.parseInt(DatabricksJdbcUrlParams.CHUNK_READY_TIMEOUT_SECONDS.getDefaultValue()); | ||
| private com.databricks.jdbc.api.internal.IDatabricksConnectionContext connectionContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import com.databricks.jdbc.api.internal.IDatabricksConnectionContext into file. Use imports everywhere else as well..
| */ | ||
| public TelemetryCollector getOrCreateCollector(IDatabricksConnectionContext context) { | ||
| String key = | ||
| (context != null && context.getConnectionUuid() != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for the context or connection uuid to be null? If both are always non-null, then we should enforce that in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am skeptical of removing this because of ThreadLocal usage. Check the internal outage here : [Link]
| this.executorService = executorService; | ||
| this.scheduledExecutorService = | ||
| Executors.newSingleThreadScheduledExecutor(createSchedulerThreadFactory()); | ||
| this.scheduledExecutorService = scheduledExecutorService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a flush is slow, does it block flush for other statements?
Description
flushIntervalMillisconfig same across both telemetry clients (un-auth and auth)Testing
Additional Notes to the Reviewer
host-A, closing connections 1-4 does nothing (just decrements refCount). Only when you close connection 5 (the last one) does the flush occur, sending all accumulated telemetry from all 5 connections.NO_CHANGELOG=true