config: correct value of expected retention

[tchaikov]

the config's type of "retention.ms" is defined in topic_properties, like

tristate<std::chrono::milliseconds> retention_duration{std::nullopt};

and we use describe_as_string() to format the value of config description, and describe_as_string() in turn uses fmt::format() to stringify the value of config's value. in this case, the type of "retention.ms" is std::chrono::milliseconds, which is formatted like 1234ms if its value is std::chrono::milliseconds(1234) in fmtlib v6.x, v7.x, v8.x and v.9.0.

in this change, the expected value is changed from "1234" to fmt::format("{}", 1234ms) to more future-proof, in case fmtlib changes its formatting in future.

for the same reason, the calls like config::shard_local_cfg().delete_retention_ms().value_or(-1ms).count() are also changed to config::shard_local_cfg().delete_retention_ms().value_or(-1ms).

Signed-off-by: Kefu Chai [email protected]

Cover letter

Describe in plain language the motivation (bug, feature, etc.) behind the change in this PR and how the included commits address it.

Fixes #ISSUE-NUMBER, Fixes #ISSUE-NUMBER, ...

Backport Required

• [ ] not a bug fix
• [ ] papercut/not impactful enough to backport
• [ ] v22.2.x
• [ ] v22.1.x
• [ ] v21.11.x

UX changes

Describe in plain language how this PR affects an end-user. What topic flags, configuration flags, command line flags, deprecation policies etc are added/changed.

Release notes

[tchaikov]

@LenaAn hi Lena, could you please take a look? see https://godbolt.org/z/rEedzG7WG for a minimal reproducer to demonstrate the behavior of fmt::format().

i am able to reproduce this test failure using ctest --output-on-failure -R coproc_fixture_unstable_rpunit

[LenaAn]

Hi @tchaikov , thanks so much for your contribution!

This change is to src/v/kafka/server/tests/alter_config_test.cc, which is a part of test_kafka_server_fixture, what's with coproc_fixture_unstable_rpunit?

I don't quite understand how this test works now if std::chrono::milliseconds(1234) is formatted like 1234ms and we expect 1234?

[tchaikov]

Hi @tchaikov , thanks so much for your contribution!

This change is to src/v/kafka/server/tests/alter_config_test.cc, which is a part of test_kafka_server_fixture, what's with coproc_fixture_unstable_rpunit?

@LenaAn sorry, i was looking at a wrong terminal. yeah, should be ctest --output-on-failure -R test_kafka_server_fixture_rpunit.

I don't quite understand how this test works now if std::chrono::milliseconds(1234) is formatted like 1234ms and we expect 1234?

no, we don't expect "1234". what we expect is the output of fmt::format("{}", 1234ms).

$ bin/test_kafka_server_fixture_rpunit 2>&1  | tee foo.out
$ grep 1234 foo.out
INFO  2022-08-02 20:11:56,559 [shard 0] cluster - topics_frontend.cc:82 - Create topics {{configuration: { topic: {ns: {kafka}, topic: {topicC}}, partition_count: 3, replication_factor: 1, properties: { compression: {nullopt}, cleanup_policy_bitflags: {delete}, compaction_strategy: {nullopt}, retention_bytes: {1234567}, retention_duration_ms: {}, segment_size: {7654321}, timestamp_type: {nullopt}, recovery_enabled: {nullopt}, shadow_indexing: {nullopt}, read_replica: {nullopt}, read_replica_bucket: {nullopt} }}, custom_assignments: {}}, {configuration: { topic: {ns: {kafka}, topic: {topicD}}, partition_count: 3, replication_factor: 1, properties: { compression: {nullopt}, cleanup_policy_bitflags: {delete}, compaction_strategy: {nullopt}, retention_bytes: {1234567}, retention_duration_ms: {}, segment_size: {7654321}, timestamp_type: {nullopt}, recovery_enabled: {nullopt}, shadow_indexing: {nullopt}, read_replica: {nullopt}, read_replica_bucket: {nullopt} }}, custom_assignments: {}}}
../src/v/kafka/server/tests/alter_config_test.cc(226): fatal error: in "test_alter_single_topic_config": critical check cfg_it->value == value has failed [{1234ms} != 1234]
../src/v/kafka/server/tests/alter_config_test.cc(226): fatal error: in "test_alter_multiple_topics_config": critical check cfg_it->value == value has failed [{1234ms} != 1234]
../src/v/kafka/server/tests/alter_config_test.cc(226): fatal error: in "test_alter_configuration_should_override": critical check cfg_it->value == value has failed [{1234ms} != 1234]
../src/v/kafka/server/tests/alter_config_test.cc(226): fatal error: in "test_incremental_alter_config": critical check cfg_it->value == value has failed [{1234ms} != 1234]
../src/v/kafka/server/tests/alter_config_test.cc(226): fatal error: in "test_incremental_alter_config_remove": critical check cfg_it->value == value has failed [{1234ms} != 1234]
../src/v/kafka/server/tests/alter_config_test.cc(226): fatal error: in "test_alter_config_does_not_rewrite_data_policy": critical check cfg_it->value == value has failed [{1234ms} != 1234]

[LenaAn]

hmmm I can't reproduce it locally, the test looks to pass I'm trying to understand if it is a bug fix and needs a backport

@tchaikov can you tell me the steps to reproduce the issue?

[tchaikov]

@tchaikov can you tell me the steps to reproduce the issue?

sure, will get back to you with the steps early tonight.

[tchaikov]

@LenaAn i was testing 8de704b87b6dfa23d9c0dbd28fbaadb02c254809 + #5743

$ clang --version
clang version 14.0.5 (Red Hat 14.0.5-1.el9)
$ cmake ..  -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=lld -GNinja
$ ninja test_kafka_server_fixture_rpunit
$ ctest --output-on-failure -R test_kafka_server_fixture_rpunit

[tchaikov]

@LenaAn ping?

[dotnwat]

Failure is https://github.com/redpanda-data/redpanda/issues/5276