You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@iggy.apache.org by "seokjin0414 (via GitHub)" <gi...@apache.org> on 2026/01/05 07:56:31 UTC

[PR] test(server): add API tests for message retrieval [iggy]

seokjin0414 opened a new pull request, #2537:
URL: https://github.com/apache/iggy/pull/2537

    ## Summary
   
     - Add 6 new API tests for message retrieval to `consumer_timestamp_polling_scenario.rs`
     - Tests cover offset-based polling, timestamp-based polling, and message content verification
     - All 4 transports (TCP, QUIC, HTTP, WebSocket) pass
   
     ## New Tests
   
     | Test | Description |
     |------|-------------|
     | `test_offset_from_middle()` | Poll from middle offset (1000) and verify remaining messages |
     | `test_offset_beyond_end()` | Poll beyond last offset returns empty result |
     | `test_timestamp_from_middle()` | Poll from middle timestamp returns only later messages |
     | `test_timestamp_future()` | Poll with future timestamp returns empty result |
     | `test_message_content_verification()` | Verify message ID and payload integrity |
     | `test_message_headers_verification()` | Verify user headers (string, number, bool) |
   
     ## Test Results
   
     test server::general::matrix::tcp_consumer_timestamp_polling_scenario_expects ... ok
     test server::general::matrix::websocket_consumer_timestamp_polling_scenario_expects ... ok
     test server::general::matrix::quic_consumer_timestamp_polling_scenario_expects ... ok
     test server::general::matrix::http_consumer_timestamp_polling_scenario_expects ... ok
   
     test result: ok. 4 passed; 0 failed
   
     ## Notes
   
     - Existing internal tests (`get_by_offset.rs`, `get_by_timestamp.rs`) are preserved as requested
     - New tests are added as extra API-level tests
   
     Closes #1657


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "seokjin0414 (via GitHub)" <gi...@apache.org>.

seokjin0414 commented on PR #2537:
URL: https://github.com/apache/iggy/pull/2537#issuecomment-3724271343

   @hubcio I re-read the issue
   - Maintain the same test_matrix parameters from existing internal tests (get_by_offset.rs, get_by_timestamp.rs) - message size, batch size, segment size, cache indexes, etc.
   - Add new tests using real server + SDK API
   Is this correct?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "hubcio (via GitHub)" <gi...@apache.org>.

hubcio commented on PR #2537:
URL: https://github.com/apache/iggy/pull/2537#issuecomment-3759610880

   Have you joined our discord?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "hubcio (via GitHub)" <gi...@apache.org>.

hubcio commented on code in PR #2537:
URL: https://github.com/apache/iggy/pull/2537#discussion_r2672054981


##########
core/integration/tests/server/scenarios/consumer_timestamp_polling_scenario.rs:
##########
@@ -31,10 +32,19 @@ pub async fn run(client_factory: &dyn ClientFactory) {
     let client = create_client(client_factory).await;
     login_root(&client).await;
 
+    // Existing tests

Review Comment:
   please dont put such useless comments into code



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "hubcio (via GitHub)" <gi...@apache.org>.

hubcio commented on PR #2537:
URL: https://github.com/apache/iggy/pull/2537#issuecomment-3728550608

@seokjin0414

The tests you added are functional and work correctly, but they don't address the core goal of #1657. Let me clarify what's needed.

The existing internal tests (`get_by_offset.rs`, `get_by_timestamp.rs`) use a test matrix with **432 parameter combinations**:

- Message sizes: 50B, 1KB, 20KB
- Batch patterns: small/medium/large/very_large
- Segment sizes: 10B, 200B, 10MB
- Cache index configs: None, All, OpenSegment
- Messages required to save: 1, 24, 1000, 10000

Your PR uses fixed hardcoded values (`MESSAGES_COUNT = 2000`, `BATCH_LENGTH = 100`) and doesn't vary any of these parameters. The whole point of the issue is to get the samecomprehensive coverage through the public API.

What's needed:
1. Parameterized tests (or a loop-based approach to reduce server starts as mentioned in the issue)
2. Variable message payload sizes matching the original tests
3. Adjustable server configuration for segment size, cache indexes, flush thresholds
4. Same batch patterns as the internal tests

Keep in mind that if you do it one-to-one, we'll spawn around 1300 servers and the test time would be in hours, so you have to optimize it (reuse iggy-servers across multiple testcases).

Let me know if you have questions, happy to help on Discord.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "hubcio (via GitHub)" <gi...@apache.org>.

hubcio merged PR #2537:
URL: https://github.com/apache/iggy/pull/2537


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "seokjin0414 (via GitHub)" <gi...@apache.org>.

seokjin0414 commented on PR #2537:
URL: https://github.com/apache/iggy/pull/2537#issuecomment-3764914041

   @hubcio Yes, I'm already on Discord.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "seokjin0414 (via GitHub)" <gi...@apache.org>.

seokjin0414 commented on code in PR #2537:
URL: https://github.com/apache/iggy/pull/2537#discussion_r2702098823


##########
core/integration/tests/server/scenarios/get_messages_by_offset_api.rs:
##########
@@ -0,0 +1,424 @@
+/* Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use super::PARTITION_ID;
+use bytes::BytesMut;
+use iggy::prelude::*;
+use integration::test_server::{ClientFactory, login_root};
+use std::collections::HashMap;
+use std::str::FromStr;
+
+fn small_batches() -> Vec<u32> {
+    vec![3, 4, 5, 6, 7]
+}
+
+fn medium_batches() -> Vec<u32> {
+    vec![10, 20, 30, 40]
+}
+
+fn large_batches() -> Vec<u32> {
+    vec![100, 200, 300, 400]
+}
+
+fn very_large_batches() -> Vec<u32> {
+    vec![500, 1000, 1500, 1000]

Review Comment:
   Fixed in c68aff07. Thanks for catching this



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "seokjin0414 (via GitHub)" <gi...@apache.org>.

seokjin0414 commented on code in PR #2537:
URL: https://github.com/apache/iggy/pull/2537#discussion_r2702098689


##########
core/integration/tests/server/scenarios/consumer_timestamp_polling_scenario.rs:
##########


Review Comment:
   Restored in c3d4cf3b. Sorry for the confusion.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "seokjin0414 (via GitHub)" <gi...@apache.org>.

seokjin0414 commented on PR #2537:
URL: https://github.com/apache/iggy/pull/2537#issuecomment-3733982081

   @ hubcio
   I Refactored, Could you check if this direction is correct?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "hubcio (via GitHub)" <gi...@apache.org>.

hubcio commented on code in PR #2537:
URL: https://github.com/apache/iggy/pull/2537#discussion_r2690932773


##########
core/integration/tests/server/scenarios/consumer_timestamp_polling_scenario.rs:
##########


Review Comment:
   please dont remove this test, it uses high level consumers, its unrelated to your test



##########
core/integration/tests/server/scenarios/get_messages_by_offset_api.rs:
##########
@@ -0,0 +1,424 @@
+/* Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use super::PARTITION_ID;
+use bytes::BytesMut;
+use iggy::prelude::*;
+use integration::test_server::{ClientFactory, login_root};
+use std::collections::HashMap;
+use std::str::FromStr;
+
+fn small_batches() -> Vec<u32> {
+    vec![3, 4, 5, 6, 7]
+}
+
+fn medium_batches() -> Vec<u32> {
+    vec![10, 20, 30, 40]
+}
+
+fn large_batches() -> Vec<u32> {
+    vec![100, 200, 300, 400]
+}
+
+fn very_large_batches() -> Vec<u32> {
+    vec![500, 1000, 1500, 1000]

Review Comment:
   typo, last element should be probably 2000



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] test(server): add API tests for message retrieval [iggy]

Posted by "hubcio (via GitHub)" <gi...@apache.org>.

hubcio commented on PR #2537:
URL: https://github.com/apache/iggy/pull/2537#issuecomment-3723519420

   hi, tests that you added are OK but that wasn't the goal.
   please re-read the issue, mainly:
   `We should maintain the same test matrix parameters (message sizes, caching options, etc.)`
   
   you didn't do that. if you have any questions please ask on discord.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org