Version 1.0.0 of the SNSExtendedClient #1
Conversation
aws-rizi
force-pushed
the
rizi-dev
branch
3 times, most recently
from
a4a69ef to
31827f7
Compare
|
|
||
| /** | ||
| * <p> | ||
| * Sends a message to an Amazon SNS topic or sends a text message (SMS message) directly to a phone number. |
There was a problem hiding this comment.
I think this is misleading since the extended client will only handle SQS endpoints
There was a problem hiding this comment.
For now SNSExtended only works with SQSExtended but it might be enriched to support other endpoints in future. Do you think I should remove this?
There was a problem hiding this comment.
Since right now the extended client only supports SQS I think we should make this clear in the java doc. In the future, when we support other endpoint we may update the documentation.
| */ | ||
| public AmazonSNSExtendedClient(AmazonSNS snsClient, PayloadStorageConfiguration payloadStorageConfiguration) { | ||
| super(snsClient); | ||
| this.payloadStorageConfiguration = new PayloadStorageConfiguration(payloadStorageConfiguration); |
There was a problem hiding this comment.
Why do you think the copy constructor is needed here?
There was a problem hiding this comment.
I removed making copy.
I think we do not need that since we do not do any modification on received object.
| private PayloadStore payloadStore; | ||
| private PayloadStorageConfiguration payloadStorageConfiguration; | ||
|
|
||
| public AmazonSNSExtendedClient(AmazonSNS amazonSNSToBeExtended) { |
There was a problem hiding this comment.
What's going to happen if the client object is used and the payloadStorageConfiguration is not initialized? My guess is will be getting a couple of NPE all over the code.
I believe we should remove this constructor or set a default configuration (if possible).
| totalMsgAttributesSize += Util.getStringSizeInBytes(stringVal); | ||
| } | ||
|
|
||
| ByteBuffer binaryVal = entryVal.getBinaryValue(); |
There was a problem hiding this comment.
We can change the code to
String stringVal = entryVal.getStringValue();
if (stringVal != null) {
totalMsgAttributesSize += Util.getStringSizeInBytes(stringVal);
} else {
totalMsgAttributesSize += entryVal.getBinaryValue().array().length;
}
But if entryVal.getBinaryValue() is null then it will throw a null exception. but I think the current code is fine.
| } | ||
| } | ||
|
|
||
| private void checkSizeOfMessageAttributes(Map<String, MessageAttributeValue> messageAttributes) { |
There was a problem hiding this comment.
Change this method name to validateMessageAttributesAndCalculate size and return the message size instead of void. This will allow to use the already calculated message size in the isLarge method, avoiding the calculation again
| } | ||
| } | ||
|
|
||
| private boolean isLarge(PublishRequest publishRequest) { |
There was a problem hiding this comment.
Add an extra param for the size of messages attributes, already calculated in the attributes validation
| messageAttributeValue.setStringValue(messageContentSize.toString()); | ||
| publishRequest.addMessageAttributesEntry(SQSExtendedClientConstants.RESERVED_ATTRIBUTE_NAME, messageAttributeValue); | ||
|
|
||
| checkSizeOfMessageAttributes(publishRequest.getMessageAttributes()); |
There was a problem hiding this comment.
We should just add the size of the new attribute to the already calculated size avoid the calculation again
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.