cdrs-async icon indicating copy to clipboard operation
cdrs-async copied to clipboard

there may be a Bug with paging.

Open thegenius opened this issue 5 years ago • 12 comments

`let mut loop_index = 0; let mut paging_state = None; loop { println!("loop index: {:?}", &loop_index); println!("pagin state: {:?}", &paging_state); let params = page_request.build_query_params(paging_state, None); let body: ResponseBody = scylladb .query_body(&page_request.row_query_str, params) .await .unwrap();

        let mut rows_cnt = 0;
        match &body {
            ResponseBody::Result(res) => {
                match res {
                    ResResultBody::Rows(rows) => {
                        rows_cnt = rows.rows_count;
                    },
                    _ => ()
                }
            },
            _ => (),
        }
        if rows_cnt == 0 {
            return Ok(PageResult {
                page_size: page_request.page_size,
                page_index: page_request.page_index,
                total: 0,
                data: Vec::new(),
            });
        }

        paging_state = body.as_rows_metadata().unwrap().paging_state;
        if page_request.page_index == loop_index {
            let rows = body.into_rows().unwrap();
            let mut result: Vec<K> = Vec::new();
            for row in rows {
                result.push(K::try_from_row(row).expect("row transform error"));
            }
            // dbg!(rows);
            // dbg!(paging_state);
            return Ok(PageResult {
                page_size: page_request.page_size,
                page_index: page_request.page_index,
                total: 0,
                data: result,
            });
        } else {
            loop_index = loop_index + 1;
        }
    }`

thegenius avatar Apr 16 '20 13:04 thegenius

Query with QueryParams will block when page_size is other than 1. This can checked with very simple code

thegenius avatar Apr 16 '20 13:04 thegenius

@thegenius thanks for the reporting. 👍 This is definitely something worth to check and fix.

AlexPikalov avatar Apr 16 '20 17:04 AlexPikalov

@thegenius

Perhaps you may want to check if the server has more page by using response metadata instead of

if rows_cnt == 0 {
  return
}

Please check pager implementation of the sync version https://github.com/AlexPikalov/cdrs/blob/master/src/cluster/pager.rs#L115.

With that logic in place you'd need to add an exit condition to the end of the loop step as in the sync example

AlexPikalov avatar Apr 17 '20 19:04 AlexPikalov

Nevertheless pager helper is something that is definitely missed in async version.

AlexPikalov avatar Apr 17 '20 19:04 AlexPikalov

I made a simple example, the query_body function will block when params is: QueryParams { consistency: One, flags: [ PageSize, ], with_names: None, values: None, page_size: Some( 2, ), paging_state: None, serial_consistency: None, timestamp: None, }

query_body implement as below:

`async fn query_body(&mut self, query_str: &str, params: QueryParams) ->Result<ResponseBody> {

    println!("before query body");
    dbg!(&params);
    return match self.session.as_mut().query_with_params(&query_str, params).await {
        Ok(result) => match result.get_body() {
            Ok(body) => {
                println!("query success");
                dbg!(&body);
                return Ok(body)
            },
            Err(e) => {
                println!("query error");
                bail!(CqlDatabaseError::InvalidStmt {
                    msg: "error stmt".to_owned()
                })
            },
        },
        Err(e) => {
            println!("query error");
            bail!(CqlDatabaseError::InvalidStmt {
                msg: "error stmt".to_owned()
            })
        }
    };
}`

there are 4 rows in the table, and when page_size set to 1, the query_body function will not block;

thegenius avatar Apr 18 '20 02:04 thegenius

OS: macOS Database: ScyllaDB 3.2 Rust version: 1.41.0

thegenius avatar Apr 18 '20 03:04 thegenius

👍 thanks! I'll check this out!

AlexPikalov avatar Apr 18 '20 05:04 AlexPikalov

@krojew have you fixed this issue in your fork?

Jasperav avatar Dec 15 '20 18:12 Jasperav

@Jasperav to be honest - I don't know if this bug even exists in cdrs-tokio.

krojew avatar Dec 15 '20 18:12 krojew

@krojew well I guess if the tests are green after merging https://github.com/AlexPikalov/cdrs/pull/337 in your fork, there shouldn't be a paging bug (since I included a test in that PR that tests the paging functionality)

Jasperav avatar Dec 15 '20 18:12 Jasperav

@Jasperav I can confirm new tests pass.

krojew avatar Dec 15 '20 18:12 krojew

@thegenius if you can switch over to krojew's fork and it also works for you, please close this issue

Jasperav avatar Dec 15 '20 18:12 Jasperav