Skip to content

Conversation

@RamKannekanti
Copy link
Contributor

RAD-6204 Update Mango Configuration Data Source To Support New Locators

RAD-6206 Update Mango Configuration Data Source To Support Publishers

@RamKannekanti RamKannekanti requested a review from aasbra December 1, 2025 19:04
Copy link
Contributor

@aasbra aasbra left a comment

Choose a reason for hiding this comment

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

I've added comments that should be addressed before the PR can be merged. Please let me know if you have any questions.

}
}

function processSystemMetrisLocatorType(locatorValue){
Copy link
Contributor

Choose a reason for hiding this comment

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

processSystemMetrisLocatorType should be processSystemMetricsLocatorType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected

return 'undefined';
}
}catch(err){
LOG.error('Publisher Locator testing' + ' ' + err.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only have "testing" in the logs during testing, that should not be in the final PR. Also, this method is a generic method for retrieving JSON values, it's not specific to Publishers. The error log entry should not reference Publisher. It should reference an error when retrieving a JSON value. It would also be good to include the jsonObject and path data to help with troubleshooting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected jsonObject and path added in log entry

}
}
} else {
LOG.debug("empty object receioved.");
Copy link
Contributor

Choose a reason for hiding this comment

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

receioved should be received

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

return this.fetchedLocatorValue;
}

function getValueBackup(jsonObject,path){
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method removed, it is only backup of existing method

return this.fetchedLocatorValue;
}

function getTestJson(){
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this method? If this is from testing, it should not be included in the final PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method removed . It provides test json data for testing.

continue;
}

if(locatorType === 'publisher') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add logging locatorParam1 inside the publisher method and remove it from this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has been fixed.

}

function processSystemMetrisLocatorType(locatorValue){
return monitoredValuesHashList.get(locatorValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to provide some logging if the locatorValue is not found. All the other methods include this type of logging, this one should not be different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

locatorValue not found logging info added

var AbstractPublisherModelObj = RestModelMapperInstance.map(PublisherVO, AbstractPublisherModel.class, Common.getUser());
var PublisherJsonString = mapperInstance.writerWithDefaultPrettyPrinter().writeValueAsString(AbstractPublisherModelObj);

return getPublisherLocatorTypePropertyValue(PublisherJsonString,locatorValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to provide some logging if the locatorValue is not found. All the other methods include this type of logging, this one should not be different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has been fixed


//get the bean named restObjectMapper and make sure it is of type ObjectMapper
var mapperInstance = Common.getBean(ObjectMapper.class,"restObjectMapper");
var PublisherVO= PublisherService.get(locatorParam1);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add a check to confirm locatorParam1 returns a Publisher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added. please check line 280-292

return getPublisherLocatorTypePropertyValue(PublisherJsonString,locatorValue);

} catch(err){
LOG.error('Publisher locatorParam1: ' + locatorParam1 + ' ' + err.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should include the locatorValue in this log as well to help with troubleshooting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes added

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.

3 participants