Skip to content

Commit

Permalink
fix: allow http client retry when lost response with next_uri=/final.
Browse files Browse the repository at this point in the history
  • Loading branch information
youngsofun committed May 23, 2024
1 parent b80bb5d commit 986748d
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 8 deletions.
2 changes: 1 addition & 1 deletion src/query/service/src/servers/http/v1/json_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use databend_common_formats::field_encoder::FieldEncoderValues;
use databend_common_io::prelude::FormatSettings;
use serde_json::Value as JsonValue;

#[derive(Debug, Clone)]
#[derive(Debug, Clone, Default)]
pub struct JsonBlock {
pub(crate) data: Vec<Vec<JsonValue>>,
}
Expand Down
11 changes: 7 additions & 4 deletions src/query/service/src/servers/http/v1/query/page_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,13 @@ impl PageManager {
Ok(page)
} else {
// when end is set to true, client should recv a response with next_url = final_url
Err(ErrorCode::Internal(format!(
"expect /final from client, got /page/{}.",
page_no
)))
// but the response may be lost and client will retry,
// we simply return an empty page.
let page = Page {
data: JsonBlock::default(),
total_rows: self.total_rows,
};
Ok(page)
}
} else if page_no + 1 == next_no {
// later, there may be other ways to ack and drop the last page except collect_new_page.
Expand Down
14 changes: 11 additions & 3 deletions src/query/service/tests/it/servers/http/http_query_handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,14 +305,22 @@ async fn test_simple_sql() -> Result<()> {
assert_eq!(result.state, ExecuteStateKind::Succeeded, "{:?}", result);
}

// get page not expected
// client retry
let page_1_uri = make_page_uri(query_id, 1);
let response = get_uri(&ep, &page_1_uri).await;
let (_, result) = get_uri_checked(&ep, &page_0_uri).await?;
assert_eq!(status, StatusCode::OK, "{:?}", result);
assert!(result.error.is_none(), "{:?}", result);
assert_eq!(result.data.len(), 0, "{:?}", result);
assert_eq!(result.next_uri, Some(final_uri.clone()), "{:?}", result);

// get page not expected
let page_2_uri = make_page_uri(query_id, 2);
let response = get_uri(&ep, &page_2_uri).await;
assert_eq!(response.status(), StatusCode::NOT_FOUND, "{:?}", result);
let body = response.into_body().into_string().await.unwrap();
assert_eq!(
body,
r#"{"error":{"code":"404","message":"expect /final from client, got /page/1."}}"#
r#"{"error":{"code":"404","message":"wrong page number 2"}}"#
);

// final
Expand Down

0 comments on commit 986748d

Please sign in to comment.