SNOW-950923 queryid for put/get statement #828
Conversation
…found to provide QueryID (potential BCR)
sfc-gh-mhofman
force-pushed
the
SNOW-950923-queryid-for-putget-statement
branch
from
4868359 to
13d1106
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #828 +/- ##
==========================================
- Coverage 83.56% 83.48% -0.08%
==========================================
Files 89 89
Lines 9155 9197 +42
Branches 837 843 +6
==========================================
+ Hits 7650 7678 +28
- Misses 1275 1287 +12
- Partials 230 232 +2 ☔ View full report in Codecov by Sentry. |
| { | ||
| download(); | ||
| Logger.Error("Error while transferring file(s): " + e.Message); |
There was a problem hiding this comment.
it made sense to wrap any exception here like FileNotFound or DirectoryNotFoundException into SnowflakeDbException as in
https://github.com/snowflakedb/snowflake-connector-net/pull/825/files#diff-db3c83b5b538ba4849e37bb0565ec20d836891f35dd60156da30501ea34ec6bfR231-R245
to be able to provide QueryId within exception. However,
it would introduce possibly some changes to the application which might already be catching only those types of exceptions and not SnowflakeDbException or exception in general.
So a change in SFStatement.cs is enough to provide query id in case file transfer error throws anything here.
| @@ -356,6 +356,11 @@ internal SFBaseResultSet Execute(int timeout, string sql, Dictionary<string, Bin | |||
| bindings, | |||
| describeOnly); | |||
|
|
|||
| logger.Debug("PUT/GET queryId: " + (response.data != null ? response.data.queryId : "Unknown")); | |||
|
|
|||
| if (response.data != null) | |||
There was a problem hiding this comment.
Since query id is provided by the above ExecuteHelper upfront (before we start get/put file operation) we reuse it even before fileTransferAgent throws any possible error.
It's a simpler approach then in #825 @sfc-gh-knozderko
cc @sfc-gh-dszmolka
| // Checking if query Id is provided on the command level as well | ||
| Assert.AreEqual(queryId, ((SnowflakeDbCommand)command).GetQueryId()); | ||
| // Check file status | ||
| Assert.AreEqual(expectedStatus.ToString(), |
There was a problem hiding this comment.
now all these asserts don't work, because exception is caught and not rethrown
There was a problem hiding this comment.
if you refer to the exception from reader.Read how not rethrowing exception would affect asserts which are skipped in case of this exception?
Those asserts take place if there is a successful scenario.
Also all the tests pass right now. If there was an issue we would see some failures
There was a problem hiding this comment.
command.ExecuteReader() does not throw any exception, but reader.Read() return false.
In that case previously the test would fail. Currently the test will pass.
I would prefer to add try/catch only around execute and return the function immediately and not putting these asserts into try/catch block, because it makes some scenarios untested.
sfc-gh-mhofman
changed the title
|
Closing as this change does not provide query id on the exception level but only at the statement level. |
Description
Please explain the changes you made here.
Checklist
dotnet test)