FMWK-468 Allow send key classes to use PK instead of Bin

[tim-aero]

Added functionality to allow the record key to be stored in the database key field rather than requiring an explicit bin.

[roimenashe]

@tim-aero
Did the Java Object Mapper support key as a bin previously to this change?
PK (stored in the record's metadata) is definitely the way to go which btw aligns with our Spring Data Aerospike implementation.

Should we even keep the previous "key as bin" approach? is it done only for backward compatibility? if so we can maybe mark "key as bin" as deprecated with warning of use PK instead and remove this flag in later versions (In Spring Data Aerospike we had the same problem and we removed the "key as bin" without deprecation but it's not ideal from backward perspective)

Do we use this "key as bin" when reading a record? (to map to the POJO?) - we can try map it from the PK if exists and if not from the "key as bin" that way we can support reads for both approaches (in case we will change it to PK without supporting "key as bin" and still being able to read the old records).

WDYT?

[tfaulkes]

Thanks Roi. Prior to this change, the object mapper would always store the key in a column, irrespective of whether sendKey was true or not. There are multiple situations where the key is used like: - loading deferred objects - using getters and setters for the primary key (for example making a compound key from multiple fields) - loading subordinate referenced objects, both lazily and normal. I think we should tread carefully where the key is used, but I like the idea that if sendKey is true then only the PK should be used as the key by default. The new flags should stay in case there are situations where behavior doesn’t work correctly unless the key is explicitly stored in a column. For example, if some data is populated into the set by another application without sendKey = true and SI scans are required. I don’t think there’s anything to deprecate though. We still need @AerospikeKey and this flag didn’t exist before. Maybe we document it as currently defaulting to false, but with the intention that it will change to default to true in the future, but only if “sendKey” is true. Thoughts? On Jun 16, 2024, at 02:10, Roi Menashe ***@***.***> wrote:  @tim-aero<https://github.com/tim-aero> Did the Java Object Mapper support key as a bin previously to this change? PK (stored in the record's metadata) is definitely the way to go which btw aligns with our Spring Data Aerospike implementation. Should we even keep the previous "key as bin" approach? is it done only for backward compatibility? if so we can maybe mark "key as bin" as deprecated with warning of use PK instead and remove this flag in later versions (In Spring Data Aerospike we had the same problem and we removed the "key as bin" without deprecation but it's not ideal from backward perspective) Do we use this "key as bin" when reading a record? (to map to the POJO?) - we can try map it from the PK if exists and if not from the "key as bin" that way we can support reads for both approaches (in case we will change it to PK without supporting "key as bin" and still being able to read the old records). WDYT? — Reply to this email directly, view it on GitHub<#160 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABB63E4IYW3QZPNLXGO6OFTZHVB7RAVCNFSM6AAAAABJLCVCE6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZRGE4DENBYGY>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[roimenashe]

@tim-aero
Few things:

1. Refactored and cleaned up a bit so make sure you are pulling before making any change

2. We consulted internally within our team regarding the style of the new flag and we have a small suggestion:

• Instead of storeInPkOnly, let’s reverse that and change to storeAsBin which by default will be set to true (in order not to break compatibility)
• Add validation, in case sendKey is false (which is the default) then we want to prevent the user from writing with storeAsBin = false since we won’t be able to read and map the record back due to not having the original key (the same issue could be encountered in the existing proposal as well).

[tim-aero]

Hi Roi,

Yes, I like the storeAsBin option over the storeInPkOnly and agree that storeAsBin should default to true to match the current situation. However, there seem to be 3 scenarios we should consider:

1. storeAsBin is false -- In this case we use the PK only if sendKey is true, and throw an exception if sendKey is false
2. storeAsBin is true -- current behaviour
3. storeAsBin is not specified -- in this case it should default to the same value as sendKey

So I'm wondering if we should create an Enum like storeKey with values IN_PK and IN_BIN, with null allowing it to default to IN_PK if sendKey is true.

There is currently a check to see if storeInPkOnly == true && sendKey == false which will throw an exception if true.

[roimenashe]

@tfaulkes
I agree with (1) - which adds the validation we talked about, (2) - current behavior in case sendKey is false, if sendKey is true we will store the key in both PK and "key as bin" but I don't see (3) the same way as you, I think if storeAsBin isn't specified it's default should be true for now to align with the same behavior as we have now and not set to the current sendKey value (which in my opinion should be 2 completely unrelated options unless both are not set and then we should throw an exception as you mentioned).

I don't like the enum idea because sendKey option already exists as part of @AerospikeRecord, if it wasn't the case we could argue about the design and maybe even always set sendKey to true behind the scenes as we do in Spring Data Aerospike and keep storeAsBin optional with true as default (just to avoid breaking the compatibility), but since it isn't the case I would go with:

1. storeAsBin is false -- In this case we use the PK only if sendKey is true, and throw an exception if sendKey is false (same as you said).
2. storeAsBin is true -- current behaviour (in case sendKey is also true we will store the key both in PK and as a bin).
3. storeAsBin isn't specified -- default should be true (regardless of sendKey value) to align with the current behavior that doesn't include storeAsBin option but always writes the key in a bin.