Skip to content

Commit

Permalink
Merge branch 'SNOW-715510' of https://github.com/snowflakedb/libsnowf…
Browse files Browse the repository at this point in the history
…lakeclient into SNOW-715510
  • Loading branch information
sfc-gh-ext-simba-jy committed Oct 11, 2024
2 parents aef38d3 + 6966872 commit fd09ef8
Show file tree
Hide file tree
Showing 12 changed files with 71 additions and 65 deletions.
5 changes: 0 additions & 5 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,6 @@ set(OPENSSL_VERSION_NUMBER 0x11100000L)
# Generates compile_commands.json file for clangd to parse.
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

if ($ENV{XP_BUILD})
message("-- Building libsnowflakeclient for XP")
add_definitions(-DLIBSFCLI_FOR_XP)
endif ()

if (MOCK)
set(MOCK_OBJECT_WRAPPER_FLAGS -Wl,--wrap=http_perform)
add_definitions(-DMOCK_ENABLED)
Expand Down
10 changes: 2 additions & 8 deletions ci/build/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ function download_build_component()
else
rm -rf $DEPENDENCY_DIR/$component_name

if [[ ! -z "$XP_BUILD" ]] ; then
src_path="$DEP_URL_PREFIX/snowflakeclient_for_xp/$component_name"
else
src_path="$DEP_URL_PREFIX/$component_name"
fi
src_path="$DEP_URL_PREFIX/$component_name"

if ! aws s3 cp --only-show-errors $src_path/$zip_file_name $ARTIFACTS_DIR; then
echo "=== build: $component_name ==="
Expand Down Expand Up @@ -68,9 +64,7 @@ function build_component()
"$component_script" -t "$build_type" "$other_args"
local component_version=$("$component_script" -v)
if [[ -z "$GITHUB_ACTIONS" ]] && [[ -n "$GIT_BRANCH" ]]; then
if [[ -z $XP_BUILD ]] ; then #upload to jenkins if not XP build
upload_to_sfc_jenkins $component_name $component_version $build_type
fi
upload_to_sfc_jenkins $component_name $component_version $build_type
if [[ "$GIT_BRANCH" == "origin/master" || "$GIT_BRANCH" == "master" ]]; then
upload_to_sfc_dev1_data $component_name $component_version $build_type
fi
Expand Down
4 changes: 0 additions & 4 deletions cpp/logger/SFLogger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,5 @@ std::string Snowflake::Client::SFLogger::getMaskedMsgVA(const char* fmt, va_list
}
}

#ifdef LIBSFCLI_FOR_XP
return std::string(buf.data());
#else
return SecretDetector::maskSecrets(std::string(buf.data()));
#endif
}
5 changes: 5 additions & 0 deletions cpp/util/SnowflakeCommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -137,5 +137,10 @@ void STDCALL sf_memory_error_handler()
exit(EXIT_FAILURE);
}

uint64 sf_get_current_time_millis()
{
return std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::system_clock::now().time_since_epoch()).count();
}

}

1 change: 1 addition & 0 deletions lib/client_int.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#define URL_PARAM_REQEST_GUID "request_guid="
#define URL_PARAM_RETRY_COUNT "retryCount="
#define URL_PARAM_RETRY_REASON "retryReason="
#define URL_PARAM_CLIENT_START_TIME "clientStartTime="

#define CLIENT_APP_ID_KEY "CLIENT_APP_ID"
#define CLIENT_APP_VERSION_KEY "CLIENT_APP_VERSION"
Expand Down
9 changes: 7 additions & 2 deletions lib/connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -1098,7 +1098,7 @@ uint32 STDCALL retry_ctx_next_sleep(RETRY_CONTEXT *retry_ctx) {
retry_ctx->sleep_time = get_next_sleep_with_jitter(retry_ctx->djb, retry_ctx->sleep_time, retry_ctx->retry_count);

// limit the sleep time within retry timeout
uint32 time_elapsed = time(NULL) - retry_ctx->start_time;
uint32 time_elapsed = time(NULL) - (retry_ctx->start_time / 1000);
if (time_elapsed >= retry_ctx->retry_timeout)
{
// retry timeout is checked before calling retry_ctx_next_sleep
Expand Down Expand Up @@ -1170,6 +1170,11 @@ sf_bool STDCALL retry_ctx_update_url(RETRY_CONTEXT *retry_ctx,
retry_ctx->retry_reason = 0;
}

// add client start time in milliseconds
SPRINT_TO_BUFFER(retry_context_ptr, "%s", URL_PARAM_CLIENT_START_TIME);
SPRINT_TO_BUFFER(retry_context_ptr, "%llu", retry_ctx->start_time);
SPRINT_TO_BUFFER(retry_context_ptr, "%s", URL_PARAM_DELIM);

// add request guid after retry context
request_guid_ptr = retry_context_ptr;

Expand Down Expand Up @@ -1340,4 +1345,4 @@ int64 get_retry_timeout(SF_CONNECT *sf)
int8 get_login_retry_count(SF_CONNECT *sf)
{
return (int8)get_less_one(sf->retry_on_connect_count, sf->retry_count);
}
}
7 changes: 6 additions & 1 deletion lib/connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ typedef struct RETRY_CONTEXT {
// Decorrelate Jitter is used to determine sleep time
DECORRELATE_JITTER_BACKOFF *djb;
// start time to track on retry timeout
time_t start_time;
uint64 start_time;
} RETRY_CONTEXT;

typedef struct SF_HEADER {
Expand Down Expand Up @@ -618,6 +618,11 @@ int8 get_login_retry_count(SF_CONNECT *sf);
*/
int64 get_retry_timeout(SF_CONNECT *sf);

/*
* Get current time since epoch in milliseconds
*/
uint64 sf_get_current_time_millis();

#ifdef __cplusplus
}
#endif
Expand Down
4 changes: 1 addition & 3 deletions lib/http_perform.c
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ sf_bool STDCALL http_perform(CURL *curl,
network_timeout,
djb.base, // time to sleep
&djb, // Decorrelate jitter
time(NULL)
sf_get_current_time_millis() // start time
};
time_t elapsedRetryTime = time(NULL);
RAW_JSON_BUFFER buffer = {NULL, 0};
Expand Down Expand Up @@ -321,7 +321,6 @@ sf_bool STDCALL http_perform(CURL *curl,
}

#ifndef _WIN32
#ifndef LIBSFCLI_FOR_XP
// If insecure mode is set to true, skip OCSP check not matter the value of SF_OCSP_CHECK (global OCSP variable)
sf_bool ocsp_check;
if (insecure_mode) {
Expand All @@ -335,7 +334,6 @@ sf_bool STDCALL http_perform(CURL *curl,
curl_easy_strerror(res));
break;
}
#endif
#endif

// Set chunk downloader specific stuff here
Expand Down
18 changes: 0 additions & 18 deletions scripts/_init.sh
Original file line number Diff line number Diff line change
Expand Up @@ -23,24 +23,6 @@ else
export CTEST="$(which ctest)"
fi

# for XP_BUILD try gcc82 first
if [[ -n "$XP_BUILD" ]] ; then
if which gcc-82 >& /dev/null; then
GCC="$(which gcc-82)"
GXX="$(which g++-82)"
elif which gcc82 >& /dev/null; then
GCC="$(which gcc82)"
GXX="$(which g++82)"
elif which gcc-8 >& /dev/null; then
GCC="$(which gcc-8)"
GXX="$(which g++-8)"
elif which gcc8 >& /dev/null; then
GCC="$(which gcc8)"
GXX="$(which g++8)"
fi
fi

# Otherwise try gcc52 first and default to gcc.
if [[ -z "$GCC" || -z "$GXX" ]]; then
if which gcc-52 >& /dev/null; then
GCC="$(which gcc-52)"
Expand Down
18 changes: 3 additions & 15 deletions scripts/utils.sh
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,7 @@ function upload_to_sfc_dev1_data()
local build_type=$3

local zip_file_name=$(get_zip_file_name $component_name $component_version $build_type)
if [[ ! -z "$XP_BUILD" ]] ; then
aws s3 cp --only-show-errors $UTILS_DIR/../artifacts/$zip_file_name $DEP_URL_PREFIX/snowflakeclient_for_xp/$component_name/
else
aws s3 cp --only-show-errors $UTILS_DIR/../artifacts/$zip_file_name $DEP_URL_PREFIX/$component_name/
fi
aws s3 cp --only-show-errors $UTILS_DIR/../artifacts/$zip_file_name $DEP_URL_PREFIX/$component_name/
}

function upload_to_sfc_jenkins()
Expand All @@ -130,11 +126,7 @@ function upload_to_sfc_jenkins()
local git_branch_base_name=$(echo $GIT_BRANCH | awk -F/ '{print $2}')

local zip_file_name=$(get_zip_file_name $component_name $component_version $build_type)
if [[ ! -z "$XP_BUILD" ]] ; then
local target_path=$REP_URL_PREFIX/snowflakeclient_for_xp/$component_name/$PLATFORM/$git_branch_base_name/$GIT_COMMIT/
else
local target_path=$REP_URL_PREFIX/$component_name/$PLATFORM/$git_branch_base_name/$GIT_COMMIT/
fi
local target_path=$REP_URL_PREFIX/$component_name/$PLATFORM/$git_branch_base_name/$GIT_COMMIT/
echo "=== uploading artifacts/$zip_file_name to $target_path"
aws s3 cp --only-show-errors $UTILS_DIR/../artifacts/$zip_file_name $target_path
local cmake_file_name=$(get_cmake_file_name $component_name $component_version $build_type)
Expand All @@ -159,11 +151,7 @@ function download_from_sfc_jenkins()
mkdir -p $UTILS_DIR/../artifacts
echo "$component_name $component_version $build_type"
local zip_file_name=$(get_zip_file_name $component_name $component_version $build_type)
if [[ ! -z "$XP_BUILD" ]] ; then
local source_path=$REP_URL_PREFIX/snowflakeclient_for_xp/$component_name/$PLATFORM/$git_branch_base_name/$GIT_COMMIT
else
local source_path=$REP_URL_PREFIX/$component_name/$PLATFORM/$git_branch_base_name/$GIT_COMMIT
fi
local source_path=$REP_URL_PREFIX/$component_name/$PLATFORM/$git_branch_base_name/$GIT_COMMIT
echo "=== downloading $zip_file_name from $source_path/"
aws s3 cp --only-show-errors $source_path/$zip_file_name $UTILS_DIR/../artifacts/
local cmake_file_name=$(get_cmake_file_name $component_name $component_version $build_type)
Expand Down
16 changes: 12 additions & 4 deletions tests/test_connect_negative.c
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,19 @@ void test_chunk_downloading_timeout(void** unused) {
sfstmt = snowflake_stmt(sf);

snowflake_set_attribute(sf, SF_CON_NETWORK_TIMEOUT, &timeout);
status = snowflake_query(sfstmt, sql_buf, 0);
if (status != SF_STATUS_SUCCESS) {
dump_error(&(sfstmt->error));
// sometime this could get timeout
for (int i = 0; i < 3; i++)
{
status = snowflake_query(sfstmt, sql_buf, 0);
if (status == SF_STATUS_SUCCESS) {
break;
}
}
if (status != SF_STATUS_SUCCESS)
{
fprintf(stderr, "test_chunk_downloading_timeout: query timeout after retry, skip.\n");
return;
}
assert_int_equal(status, SF_STATUS_SUCCESS);

unsigned long start_time = (unsigned long)time(NULL);
// we could have some rows returned in query response so fetch
Expand Down
39 changes: 34 additions & 5 deletions tests/test_unit_retry_context.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#define URL_NON_QUERY_WITH_GUID "http://snowflake.com/other-path?request_guid=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
#define URL_NO_GUID "http://snowflake.com/other-path"
#define GUID_LENGTH strlen("request_guid=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx")
#define CLIENT_START_TIME_LENGTH strlen("clientStartTime=xxxxxxxxxxxxx&")

#define URL_LOGIN "http://snowflake.com/session/v1/login-request"
#define URL_TOKEN "http://snowflake.com/session/token-request"
Expand Down Expand Up @@ -52,6 +53,7 @@ void test_update_other_url_with_guid(void **unused) {
assert_int_equal(strlen(urlbuf), strlen(URL_NON_QUERY_WITH_GUID));
assert_null(strstr(urlbuf, URL_PARAM_RETRY_COUNT));
assert_null(strstr(urlbuf, URL_PARAM_RETRY_REASON));
assert_null(strstr(urlbuf, URL_PARAM_CLIENT_START_TIME));
char* guid_ptr = strstr(urlbuf, URL_PARAM_REQEST_GUID);
assert_non_null(guid_ptr);
assert_int_equal(strlen(guid_ptr), GUID_LENGTH);
Expand All @@ -67,7 +69,8 @@ void test_update_query_url_with_retry_reason_disabled(void **unused) {
429, // retry reason
0, // network_timeout
1, // time to sleep
NULL // Decorrelate jitter
NULL, // Decorrelate jitter
sf_get_current_time_millis() // start time
};

sf_bool ret = retry_ctx_update_url(&retry_ctx, urlbuf, SF_BOOLEAN_FALSE);
Expand All @@ -79,6 +82,12 @@ void test_update_query_url_with_retry_reason_disabled(void **unused) {
assert_int_equal(strlen(guid_ptr), GUID_LENGTH);
*guid_ptr = '\0';

// should have client start time added
char* starttime_ptr = strstr(urlbuf, URL_PARAM_CLIENT_START_TIME);
assert_non_null(starttime_ptr);
assert_int_equal(strlen(starttime_ptr), CLIENT_START_TIME_LENGTH);
*starttime_ptr = '\0';

// should not have retry reason added
char* retryreason_ptr = strstr(urlbuf, URL_PARAM_RETRY_REASON);
assert_null(retryreason_ptr);
Expand All @@ -95,6 +104,7 @@ void test_update_query_url_with_retry_reason_disabled(void **unused) {
// recover the updated URL for testing next retry
*guid_ptr = 'r';
*retrycount_ptr = 'r';
*starttime_ptr = 'c';
retry_ctx.retry_count = 2;
retry_ctx.retry_reason = 503;

Expand All @@ -107,6 +117,12 @@ void test_update_query_url_with_retry_reason_disabled(void **unused) {
assert_int_equal(strlen(guid_ptr), GUID_LENGTH);
*guid_ptr = '\0';

// should have client start time added
starttime_ptr = strstr(urlbuf, URL_PARAM_CLIENT_START_TIME);
assert_non_null(starttime_ptr);
assert_int_equal(strlen(starttime_ptr), CLIENT_START_TIME_LENGTH);
*starttime_ptr = '\0';

// should not have retry reason added
retryreason_ptr = strstr(urlbuf, URL_PARAM_RETRY_REASON);
assert_null(retryreason_ptr);
Expand All @@ -129,9 +145,9 @@ void test_update_query_url_with_retry_reason_enabled(void **unused) {
429, // retry reason
0, // network_timeout
1, // time to sleep
NULL // Decorrelate jitter
NULL, // Decorrelate jitter
sf_get_current_time_millis() // start time
};

sf_bool ret = retry_ctx_update_url(&retry_ctx, urlbuf, SF_BOOLEAN_TRUE);
assert_int_equal(ret, SF_BOOLEAN_TRUE);

Expand All @@ -141,6 +157,12 @@ void test_update_query_url_with_retry_reason_enabled(void **unused) {
assert_int_equal(strlen(guid_ptr), GUID_LENGTH);
*guid_ptr = '\0';

// should have client start time added
char* starttime_ptr = strstr(urlbuf, URL_PARAM_CLIENT_START_TIME);
assert_non_null(starttime_ptr);
assert_int_equal(strlen(starttime_ptr), CLIENT_START_TIME_LENGTH);
*starttime_ptr = '\0';

// should have retry count/reason added
char* retrycount_ptr = strstr(urlbuf, URL_PARAM_RETRY_COUNT);
assert_non_null(retrycount_ptr);
Expand All @@ -153,6 +175,7 @@ void test_update_query_url_with_retry_reason_enabled(void **unused) {
// recover the updated URL for testing next retry
*guid_ptr = 'r';
*retrycount_ptr = 'r';
*starttime_ptr = 'c';
retry_ctx.retry_count = 2;
retry_ctx.retry_reason = 503;

Expand All @@ -165,6 +188,12 @@ void test_update_query_url_with_retry_reason_enabled(void **unused) {
assert_int_equal(strlen(guid_ptr), GUID_LENGTH);
*guid_ptr = '\0';

// should have client start time added
starttime_ptr = strstr(urlbuf, URL_PARAM_CLIENT_START_TIME);
assert_non_null(starttime_ptr);
assert_int_equal(strlen(starttime_ptr), CLIENT_START_TIME_LENGTH);
*starttime_ptr = '\0';

// should have retry count/reason updated
retrycount_ptr = strstr(urlbuf, URL_PARAM_RETRY_COUNT);
assert_non_null(retrycount_ptr);
Expand All @@ -186,7 +215,7 @@ void test_new_retry_strategy(void **unused) {
SF_RETRY_TIMEOUT,
djb.base, // time to sleep
&djb, // Decorrelate jitter
time(NULL)
sf_get_current_time_millis() // start time
};

uint32 error_codes[SF_MAX_RETRY] = {429, 503, 403, 408, 400, 538, 525};
Expand All @@ -202,7 +231,7 @@ void test_new_retry_strategy(void **unused) {
assert_int_equal(is_retryable_http_code(error_codes[retry_count]), SF_BOOLEAN_TRUE);
next_sleep_in_secs = retry_ctx_next_sleep(&curl_retry_ctx);
// change the start time to mock the time elapsed
curl_retry_ctx.start_time -= next_sleep_in_secs;
curl_retry_ctx.start_time -= next_sleep_in_secs * 1000;
total_backoff += next_sleep_in_secs;

if (total_backoff + next_sleep_in_secs >= SF_LOGIN_TIMEOUT)
Expand Down

0 comments on commit fd09ef8

Please sign in to comment.