Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix[MQB]: add log for response message #487

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

emelialei88
Copy link
Collaborator

Log the responses in AdminSession::finalizeAdminCommand() to make it easier to read the response.

@emelialei88 emelialei88 requested a review from a team as a code owner October 29, 2024 17:15
Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good; let's add an integration test to make sure.

@pniedzielski
Copy link
Collaborator

Behavior before:

29OCT2024_22:27:08.621 (6121811968) INFO mqba_adminsession.cpp:346 fuzztest:0: Received control message: [ rId = 0 choice = [ adminCommand = [ command = "HELP" ] ] ]
29OCT2024_22:27:08.621 (6134312960) INFO mqba_application.cpp:819 Received command 'HELP' [source: 127.0.0.1~localhost:50891]

Behavior after:

29OCT2024_22:24:14.433 (6190919680) INFO mqba_adminsession.cpp:346 fuzztest:0: Received control message: [ rId = 0 choice = [ adminCommand = [ command = "HELP" ] ] ]
29OCT2024_22:24:14.433 (6203420672) INFO mqba_application.cpp:819 Received command 'HELP' [source: 127.0.0.1~localhost:50813]
29OCT2024_22:24:14.433 (6176124928) INFO mqba_adminsession.cpp:209 fuzztest:0: Send response message: [ rId = 0 choice = [ adminCommandResponse = [ text = "This process responds to the following CMD subcommands:
    HELP
        Show list of supported commands
    BROKERCONFIG DUMP
        Dump the broker's configuration
    DOMAINS DOMAIN <name> PURGE
        Purge all queues in domain 'name'
    DOMAINS DOMAIN <name> INFOS
        Show information about domain 'name' and its queues
    DOMAINS DOMAIN <name> QUEUE <queue_name> PURGE <appId>
        Purge the 'appId' in the 'queue_name' belonging to domain 'name'. Specify '*' for 'appId' for entire queue
    DOMAINS DOMAIN <name> QUEUE <queue_name> INTERNALS
        Dump the internals of 'queue_name' belonging to domain 'name'
    DOMAINS DOMAIN <name> QUEUE <queue_name> LIST [<appId>] <offset> <count>
        List information for 'count' messages starting at 'offset' from 'queue_name' for 'appId' in domain 'name'.  If 'offset' is negative, it is relative to the position just past the *last* message.  If 'count' is 'negative', print '-count' messages *preceding* the specified starting position.  If 'count' is 'UNLIMITED', print all the messages starting at and after the specified position.
    DOMAINS RECONFIGURE <domain>
        Reconfigure 'domain' by reloading its configuration from disk
    DOMAINS RESOLVER CACHE_CLEAR (<domain>|ALL)
        Clear the domain resolution cache entry of the optionally specified 'domain', or clear all domain resolution cache entries if 'ALL' is specified.
    CONFIGPROVIDER CACHE_CLEAR (<domain>|ALL)
        Clear the cached configuration entry of the optionally specified 'domain' domain, or clear all cached configuration entries if 'ALL' is specified.
    STAT SHOW
        Show statistics
    STAT SET <parameter> <value>
        Set the 'parameter' of the stat controller to 'value'
    STAT GET <parameter>
        Get the 'parameter' of the stat controller
    STAT LIST_TUNABLES
        Get the supported settable parameters for the stat controller
    CLUSTERS LIST
        List all active clusters
    CLUSTERS ADDREVERSE <clusterName> <remotePeer>
        Create a new reverse connection to 'remotePeer' about 'clusterName'
    CLUSTERS CLUSTER <name> STATUS
        Show status of cluster 'name'
    CLUSTERS CLUSTER <name> QUEUEHELPER
        Show queueHelper's internal state of cluster 'name'
    CLUSTERS CLUSTER <name> FORCE_GC_QUEUES
        Force GC all queues matching GC criteria
    CLUSTERS CLUSTER <name> STORAGE SUMMARY
        Show storage summary of cluster 'name'
    CLUSTERS CLUSTER <name> STORAGE PARTITION <partitionId> [ENABLE|DISABLE]
        Enable/disable the 'partitionId' of cluster 'name'
    CLUSTERS CLUSTER <name> STORAGE PARTITION <partitionId> SUMMARY
        Show summary of the 'partitionId' of cluster 'name'
    CLUSTERS CLUSTER <name> STORAGE DOMAIN <domain_name> QUEUE_STATUS
        Show status of queues belonging to 'domain_name' in the storage of cluster 'name'
    CLUSTERS CLUSTER <name> STORAGE REPLICATION [SET|SET_ALL] <parameter> <value>
        Set the value of the replication 'parameter' of cluster 'name'. If SET_ALL is used then set the parameter for all nodes in the cluster.
    CLUSTERS CLUSTER <name> STORAGE REPLICATION [GET|GET_ALL] <parameter>
        Get the value of the replication 'parameter' of cluster 'name'. If GET_ALL is used then get the paramter value for all nodes in the cluster.
    CLUSTERS CLUSTER <name> STORAGE REPLICATION LIST_TUNABLES
        Get the supported settable parameters for the replication of cluster 'name'
    CLUSTERS CLUSTER <name> STATE ELECTOR [SET|SET_ALL] <parameter> <value>
        Set the 'parameter' of the elector of cluster 'name' to 'value'. If SET_ALL is used then set the paramter for all nodes in the cluster.
    CLUSTERS CLUSTER <name> STATE ELECTOR [GET|GET_ALL] <parameter>
        Get the 'parameter' of the elector of cluster 'name'. If GET_ALL is used then get the parameter value for all nodes in the cluster.
    CLUSTERS CLUSTER <name> STATE ELECTOR LIST_TUNABLES
        Get the supported settable parameters for the elector of cluster 'name'
" ] ] ]

It's verbose and so we may revisit this, but for the moment this is great.

Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, small change and typo fix, but great job! Could you squash these commits into one once you've fixed these, force push, and I'll merge.

src/integration-tests/test_admin_res_log.py Outdated Show resolved Hide resolved
src/integration-tests/test_admin_res_log.py Outdated Show resolved Hide resolved
src/integration-tests/test_admin_res_log.py Show resolved Hide resolved
src/integration-tests/test_admin_res_log.py Outdated Show resolved Hide resolved
@emelialei88 emelialei88 force-pushed the fix/add_adminsession_log branch 2 times, most recently from 5332348 to 8c85392 Compare October 30, 2024 15:57
@emelialei88 emelialei88 force-pushed the fix/add_adminsession_log branch from 8c85392 to a95bedc Compare October 30, 2024 16:29
@pniedzielski pniedzielski self-requested a review October 30, 2024 18:14
Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@pniedzielski pniedzielski merged commit 8d8a0e6 into bloomberg:main Oct 30, 2024
26 checks passed
@emelialei88 emelialei88 deleted the fix/add_adminsession_log branch October 30, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants