From 14171fb035eb07ee79604fe9ecac161ff83cf3aa Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Tue, 14 Jan 2025 17:35:16 +0100 Subject: [PATCH] Remove msg_store_io_batch_size and msg_store_credit_disc_bound checks msg_store_io_batch_size is no longer used msg_store_credit_disc_bound appears to be used in the code, but I don't see any impact of that value on the performance. It should be properly investigated and either removed completely or fixed, because there's hardly any point in warning about the values configured (plus, this settings is hopefully almost never used anyway) --- deps/rabbit/BUILD.bazel | 9 --- deps/rabbit/Makefile | 4 +- deps/rabbit/app.bzl | 9 --- deps/rabbit/ct.test.spec | 1 - deps/rabbit/src/rabbit.erl | 85 ----------------------- deps/rabbit/src/rabbit_variable_queue.erl | 8 +-- deps/rabbit/test/msg_store_SUITE.erl | 52 -------------- 7 files changed, 4 insertions(+), 164 deletions(-) delete mode 100644 deps/rabbit/test/msg_store_SUITE.erl diff --git a/deps/rabbit/BUILD.bazel b/deps/rabbit/BUILD.bazel index 68d5f16da8..0ccd2c76a7 100644 --- a/deps/rabbit/BUILD.bazel +++ b/deps/rabbit/BUILD.bazel @@ -95,7 +95,6 @@ _APP_ENV = """[ %% writing to the queue index. See the setting %% queue_index_embed_msgs_below above. {msg_store_credit_disc_bound, {4000, 800}}, - {msg_store_io_batch_size, 4096}, %% see rabbitmq-server#143, %% rabbitmq-server#949, rabbitmq-server#1098 {credit_flow_default_credit, {400, 200}}, @@ -532,14 +531,6 @@ rabbitmq_integration_suite( ], ) -rabbitmq_suite( - name = "msg_store_SUITE", - size = "small", - deps = [ - "//deps/rabbit_common:erlang_app", - ], -) - rabbitmq_integration_suite( name = "peer_discovery_classic_config_SUITE", size = "large", diff --git a/deps/rabbit/Makefile b/deps/rabbit/Makefile index a720a36fce..9006727ab6 100644 --- a/deps/rabbit/Makefile +++ b/deps/rabbit/Makefile @@ -78,7 +78,6 @@ define PROJECT_ENV %% writing to the queue index. See the setting %% queue_index_embed_msgs_below above. {msg_store_credit_disc_bound, {4000, 800}}, - {msg_store_io_batch_size, 4096}, %% see rabbitmq-server#143, %% rabbitmq-server#949, rabbitmq-server#1098 {credit_flow_default_credit, {400, 200}}, @@ -192,7 +191,6 @@ SLOW_CT_SUITES := backing_queue \ health_check \ many_node_ha \ metrics \ - msg_store \ partitions \ per_user_connection_tracking \ per_vhost_connection_limit \ @@ -272,7 +270,7 @@ PARALLEL_CT_SET_2_D = queue_length_limits queue_parallel quorum_queue_member_rec PARALLEL_CT_SET_3_A = definition_import per_user_connection_channel_limit_partitions per_vhost_connection_limit_partitions policy priority_queue_recovery rabbit_fifo_prop rabbit_fifo_v0 rabbit_stream_sac_coordinator unit_credit_flow unit_queue_consumers unit_queue_location unit_quorum_queue PARALLEL_CT_SET_3_B = cluster_upgrade list_consumers_sanity_check list_queues_online_and_offline logging lqueue maintenance_mode rabbit_fifo_q PARALLEL_CT_SET_3_C = cli_forget_cluster_node feature_flags_v2 mc_unit message_containers_deaths_v2 message_size_limit metadata_store_migration -PARALLEL_CT_SET_3_D = metadata_store_phase1 metrics mirrored_supervisor msg_store peer_discovery_classic_config proxy_protocol runtime_parameters unit_stats_and_metrics unit_supervisor2 unit_vm_memory_monitor +PARALLEL_CT_SET_3_D = metadata_store_phase1 metrics mirrored_supervisor peer_discovery_classic_config proxy_protocol runtime_parameters unit_stats_and_metrics unit_supervisor2 unit_vm_memory_monitor PARALLEL_CT_SET_4_A = clustering_events rabbit_local_random_exchange rabbit_message_interceptor rabbitmq_4_0_deprecations unit_pg_local unit_plugin_directories unit_plugin_versioning unit_policy_validators unit_priority_queue PARALLEL_CT_SET_4_B = per_user_connection_tracking per_vhost_connection_limit rabbit_fifo_dlx_integration rabbit_fifo_int diff --git a/deps/rabbit/app.bzl b/deps/rabbit/app.bzl index 1708c6af45..a1576487f8 100644 --- a/deps/rabbit/app.bzl +++ b/deps/rabbit/app.bzl @@ -1073,15 +1073,6 @@ def test_suite_beam_files(name = "test_suite_beam_files"): beam = ["ebin/mirrored_supervisor.beam"], erlc_opts = "//:test_erlc_opts", ) - erlang_bytecode( - name = "msg_store_SUITE_beam_files", - testonly = True, - srcs = ["test/msg_store_SUITE.erl"], - outs = ["test/msg_store_SUITE.beam"], - app_name = "rabbit", - erlc_opts = "//:test_erlc_opts", - deps = ["//deps/rabbit_common:erlang_app"], - ) erlang_bytecode( name = "peer_discovery_classic_config_SUITE_beam_files", testonly = True, diff --git a/deps/rabbit/ct.test.spec b/deps/rabbit/ct.test.spec index 60d65d2d56..bd8d628a4b 100644 --- a/deps/rabbit/ct.test.spec +++ b/deps/rabbit/ct.test.spec @@ -61,7 +61,6 @@ , metadata_store_phase1_SUITE , metrics_SUITE , mirrored_supervisor_SUITE -, msg_store_SUITE , peer_discovery_classic_config_SUITE ]}. diff --git a/deps/rabbit/src/rabbit.erl b/deps/rabbit/src/rabbit.erl index fbf95696f1..915d18230b 100644 --- a/deps/rabbit/src/rabbit.erl +++ b/deps/rabbit/src/rabbit.erl @@ -42,9 +42,6 @@ pg_local_amqp_session/0, pg_local_amqp_connection/0]). -%% for tests --export([validate_msg_store_io_batch_size_and_credit_disc_bound/2]). - -rabbit_boot_step({pre_boot, [{description, "rabbit boot start"}]}). -rabbit_boot_step({codec_correctness_check, @@ -969,7 +966,6 @@ start(normal, []) -> print_banner(), log_banner(), warn_if_kernel_config_dubious(), - warn_if_disc_io_options_dubious(), ?LOG_DEBUG(""), ?LOG_DEBUG("== Plugins (prelaunch phase) =="), @@ -1443,87 +1439,6 @@ warn_if_kernel_config_dubious() -> true -> ok end. -warn_if_disc_io_options_dubious() -> - %% if these values are not set, it doesn't matter since - %% rabbit_variable_queue will pick up the values defined in the - %% IO_BATCH_SIZE and CREDIT_DISC_BOUND constants. - CreditDiscBound = rabbit_misc:get_env(rabbit, msg_store_credit_disc_bound, - undefined), - IoBatchSize = rabbit_misc:get_env(rabbit, msg_store_io_batch_size, - undefined), - case catch validate_msg_store_io_batch_size_and_credit_disc_bound( - CreditDiscBound, IoBatchSize) of - ok -> ok; - {error, {Reason, Vars}} -> - ?LOG_WARNING(Reason, Vars, - #{domain => ?RMQLOG_DOMAIN_GLOBAL}) - end. - -validate_msg_store_io_batch_size_and_credit_disc_bound(CreditDiscBound, - IoBatchSize) -> - case IoBatchSize of - undefined -> - ok; - IoBatchSize when is_integer(IoBatchSize) -> - if IoBatchSize < ?IO_BATCH_SIZE -> - throw({error, - {"io_batch_size of ~b lower than recommended value ~b, " - "paging performance may worsen", - [IoBatchSize, ?IO_BATCH_SIZE]}}); - true -> - ok - end; - IoBatchSize -> - throw({error, - {"io_batch_size should be an integer, but ~b given", - [IoBatchSize]}}) - end, - - %% CreditDiscBound = {InitialCredit, MoreCreditAfter} - {RIC, RMCA} = ?CREDIT_DISC_BOUND, - case CreditDiscBound of - undefined -> - ok; - {IC, MCA} when is_integer(IC), is_integer(MCA) -> - if IC < RIC; MCA < RMCA -> - throw({error, - {"msg_store_credit_disc_bound {~b, ~b} lower than" - "recommended value {~b, ~b}," - " paging performance may worsen", - [IC, MCA, RIC, RMCA]}}); - true -> - ok - end; - {IC, MCA} -> - throw({error, - {"both msg_store_credit_disc_bound values should be integers, but ~tp given", - [{IC, MCA}]}}); - CreditDiscBound -> - throw({error, - {"invalid msg_store_credit_disc_bound value given: ~tp", - [CreditDiscBound]}}) - end, - - case {CreditDiscBound, IoBatchSize} of - {undefined, undefined} -> - ok; - {_CDB, undefined} -> - ok; - {undefined, _IBS} -> - ok; - {{InitialCredit, _MCA}, IoBatchSize} -> - if IoBatchSize < InitialCredit -> - throw( - {error, - {"msg_store_io_batch_size ~b should be bigger than the initial " - "credit value from msg_store_credit_disc_bound ~b," - " paging performance may worsen", - [IoBatchSize, InitialCredit]}}); - true -> - ok - end - end. - -spec product_name() -> string(). product_name() -> diff --git a/deps/rabbit/src/rabbit_variable_queue.erl b/deps/rabbit/src/rabbit_variable_queue.erl index 894ab363aa..ff4ca40988 100644 --- a/deps/rabbit/src/rabbit_variable_queue.erl +++ b/deps/rabbit/src/rabbit_variable_queue.erl @@ -210,7 +210,7 @@ disk_read_count, disk_write_count, - io_batch_size, + io_batch_size, %% Unused. %% default queue or lazy queue mode, %% Unused. @@ -334,7 +334,7 @@ disk_read_count :: non_neg_integer(), disk_write_count :: non_neg_integer(), - io_batch_size :: pos_integer(), + io_batch_size :: 0, mode :: 'default' | 'lazy', version :: 2, unconfirmed_simple :: sets:set()}. @@ -1195,8 +1195,6 @@ init(IsDurable, IndexState, StoreState, DeltaCount, DeltaBytes, Terms, end_seq_id = NextSeqId }) end, Now = erlang:monotonic_time(), - IoBatchSize = rabbit_misc:get_env(rabbit, msg_store_io_batch_size, - ?IO_BATCH_SIZE), {ok, IndexMaxSize} = application:get_env( rabbit, queue_index_embed_msgs_below), @@ -1242,7 +1240,7 @@ init(IsDurable, IndexState, StoreState, DeltaCount, DeltaBytes, Terms, disk_read_count = 0, disk_write_count = 0, - io_batch_size = IoBatchSize, + io_batch_size = 0, mode = default, virtual_host = VHost}, diff --git a/deps/rabbit/test/msg_store_SUITE.erl b/deps/rabbit/test/msg_store_SUITE.erl deleted file mode 100644 index 8ea81d47ae..0000000000 --- a/deps/rabbit/test/msg_store_SUITE.erl +++ /dev/null @@ -1,52 +0,0 @@ -%% This Source Code Form is subject to the terms of the Mozilla Public -%% License, v. 2.0. If a copy of the MPL was not distributed with this -%% file, You can obtain one at https://mozilla.org/MPL/2.0/. -%% -%% Copyright (c) 2007-2025 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. -%% - --module(msg_store_SUITE). - --include_lib("rabbit_common/include/rabbit.hrl"). - --compile(export_all). - --define(T(Fun, Args), (catch apply(rabbit, Fun, Args))). - -all() -> - [ - parameter_validation - ]. - -parameter_validation(_Config) -> - %% make sure it works with default values - ok = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [?CREDIT_DISC_BOUND, ?IO_BATCH_SIZE]), - - %% IO_BATCH_SIZE must be greater than CREDIT_DISC_BOUND initial credit - ok = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [{4000, 800}, 5000]), - {error, _} = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [{4000, 800}, 1500]), - - %% All values must be integers - {error, _} = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [{2000, 500}, "1500"]), - {error, _} = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [{"2000", 500}, abc]), - {error, _} = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [{2000, "500"}, 2048]), - - %% CREDIT_DISC_BOUND must be a tuple - {error, _} = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [[2000, 500], 1500]), - {error, _} = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [2000, 1500]), - - %% config values can't be smaller than default values - {error, _} = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [{1999, 500}, 2048]), - {error, _} = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [{2000, 499}, 2048]), - {error, _} = ?T(validate_msg_store_io_batch_size_and_credit_disc_bound, - [{2000, 500}, 2047]).