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 clusterutil print dereference bug #578

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

emelialei88
Copy link
Collaborator

@emelialei88 emelialei88 commented Jan 21, 2025

Fixed a bug where in mqbc_clusterutil.cpp, function ClusterUtil::validateState, some shared pointers are not dereferenced correctly.

@emelialei88 emelialei88 force-pushed the drqs-177930877 branch 2 times, most recently from 1449231 to a456bee Compare January 21, 2025 23:12
@emelialei88 emelialei88 marked this pull request as ready for review January 21, 2025 23:12
@emelialei88 emelialei88 requested a review from a team as a code owner January 21, 2025 23:12
@emelialei88 emelialei88 requested a review from kaikulimu January 21, 2025 23:13
@emelialei88 emelialei88 changed the title Fix Uri ctor and clusterutil print bug Fix clusterutil print dereference bug Jan 22, 2025
Copy link
Collaborator

@kaikulimu kaikulimu left a comment

Choose a reason for hiding this comment

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

Good job! Some comments here.


// mqbc_clusterutil.t.cpp -*-C++-*-
#include <bdlb_print.h>
#include <bslstl_string.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Include bsl_string.h instead

// mqbc_clusterutil.t.cpp -*-C++-*-
#include <bdlb_print.h>
#include <bslstl_string.h>
#include <bslstl_vector.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove unused include. (Also, it would have been bsl_vector.h instead of bslstl_vector.h)

// limitations under the License.

// mqbc_clusterutil.t.cpp -*-C++-*-
#include <bdlb_print.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move all of these BDE-related includes under the // BDE section at Line 26


// validate state
bmqu::MemOutStream errorDescription;
int rc = mqbc::ClusterUtil::validateState(errorDescription,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
int rc = mqbc::ClusterUtil::validateState(errorDescription,
const int rc = mqbc::ClusterUtil::validateState(errorDescription,

// 0. Generate different primary lease Id
original.setPartitionPrimary(0, 10, 0);
reference.setPartitionPrimary(0, 9, 0);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you also add a couple correct queues to make sure we are not generating false alarms?

keyExtraQueue.begin(),
mqbu::StorageKey::e_KEY_LENGTH_BINARY);
out << " partitionId = 2 appIdInfos = [ ] "
"stateOfAssignment = 0 ]";
Copy link
Collaborator

Choose a reason for hiding this comment

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

ClusterStateQueueInfo::State is defined here. To be informative, we should print NONE instead of 0. The trick is to add the four methods print, toAscii,fromAscii and operator<< similar to how ClusterStateTableState does it.

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