aerospike / aerospike-client-java

Aerospike Java Client Library
Other
236 stars 212 forks source link

Delete command always returns 'true' indicating record existed before deletion #161

Closed jrumbinas closed 3 years ago

jrumbinas commented 4 years ago

According to javadoc delete command returns "whether record existed on server before deletion". I've recently noticed that get and delete command results are not consistent, i.e. I'd expect ISE would never be through in the example below:

var record = client.get(null, key);
var exists = client.delete(null, key);
if (record == null && exists) {
    throw new IllegalStateException();
}

Please compare response headers for both commands (Buffer.bytesToHexString(dataBuffer, 0, Command.MSG_TOTAL_HEADER_SIZE)): read: 020300000000001616000000000200000000000000000000000000000000 delete: 020300000000001616000000000000000000000000000000000000000000

BrianNichols commented 4 years ago

I can't reproduce your results. Here is my test code.

Key key = new Key("test", "set", "key");
Record record;
boolean exists;

// Test record exists case.
client.put(null, key, new Bin("name", "val"));
record = client.get(null, key);
exists = client.delete(null, key);
System.out.println("record=" + record + " exists=" + exists);

// Test record not exists case.
record = client.get(null, key);
exists = client.delete(null, key);
System.out.println("record=" + record + " exists=" + exists);

Results:

record=(gen:1),(exp:325281146),(bins:(name:val)) exists=true
record=null exists=false
jrumbinas commented 4 years ago

ok, I've found the culprit.

var policy = new WritePolicy();
policy.durableDelete = true;

var record = client.get(policy, key);
var exists = client.delete(policy, key);
System.out.println("durableDelete: record=" + record + ", exists=" + exists);

policy.durableDelete = false;
record = client.get(policy, key);
exists = client.delete(policy, key);

System.out.println("non-durableDelete: record=" + record + ", exists=" + exists);

Results:

durableDelete: record=null, exists=false
non-durableDelete: record=null, exists=true
BrianNichols commented 4 years ago

Still can't reproduce. It might be a problem on file/ssd based namespace configurations. I'm using in-memory configuration. How is your namespace configured?

BrianNichols commented 4 years ago

Ok, I reproduced it. I had to add a put() at the beginning of your code to show the problem. I will forward this to the server team.

gooding470 commented 4 years ago

jrumbinas --

Can you explain why you want to durably delete, then non-durably delete, the same record?

We had originally left the non-durable deletion of tombstones in as a back door way to (non-durably) remove tombstones, for example to mitigate accidental durable deletes and recover index capacity. We wanted a success response (as opposed to not-found) to confirm that we actually got the tombstone.

We have not yet encountered a customer doing this sequence on purpose -- you are the first. Could you give us an idea what you're doing or planning?

gooding470 commented 4 years ago

@jrumbinas --

Ok, this morning in our product meeting, we decided that going forward we will remove the back door behavior and deletes should then behave as you expect -- deleting already deleted records, regardless of the flavor of either delete, will always return "not found". Tombstones will not be removed.

I would still be interested in why you performed this particular delete sequence, e.g. whether it was for planned usage or just exhaustive testing.

jrumbinas commented 4 years ago

@gooding470 --

We wanted a success response (as opposed to not-found) to confirm that we actually got the tombstone.

I don't think that's the case though. I've tried deleting multiple times w/ durableDeletes=false and all of them returned the same response -- record exists.

Could you give us an idea what you're doing or planning?

We wrote a utility to clean up all the data written to Aerospike during test execution. It was implemented by simply collecting a list of Key passed to certain methods. We never rely on default policies, except in this case. Unlucky us, default write policy had durableDeletes set to false. That being said, we don't have a valid use-case.

gooding470 commented 4 years ago

I've tried deleting multiple times w/ durableDeletes=false and all of them returned the same response -- record exists.

Ok, that is NOT what I would expect. Deleting with durable deletes false (the default, BTW), if it encounters a tombstone left by a durably deleted record, should respond OK, and remove the tombstone. Another non-durable delete should then get "not found". (Unless a cold restart in between caused the tombstone to come back.)

@BrianNichols is this what you saw also? I had thought we got "success".

While in principle we'll be changing going forward, I'd like to understand what happened here.

WRT your utility, cleaning up all your test records with non-durable deletes should be fine, unless you're worried about them coming back after cold restarting. But just clearing records between tests without restarting should be fine. We do that too.

Clearing with durable deletes can also work, and will prevent things coming back after cold restart, but will leave tombstones around for a long time, which depending on your tests may have interesting impact. (E.g. your index memory won't be reduced, record generations won't reset, etc.)

So I want to push further here to find out what really is going on.

gooding470 commented 4 years ago

Hm ... now I'm not sure what a result of "exists" means. Is it "ok" returned from the server? Or is it the error code for "record exists"? If exists means "ok" was returned, then I do expect this behavior the first time we non-durably delete a tombstone. But not the second. Unless the record was created and durably deleted again, i.e. the whole loop was run again.

So maybe this is what we expect would have happened. Brian?

BrianNichols commented 4 years ago

@gooding470 is correct, another durable-delete returns false ("not found"). The following code illustrates this:

Key key = new Key("test", "set", "putgetkey");
WritePolicy policy = new WritePolicy();
policy.durableDelete = true;

client.put(policy, key, new Bin("bin", "val"));
Record record = client.get(policy, key);
boolean exists = client.delete(policy, key);
System.out.println("durableDelete: record=" + record + ", exists=" + exists);

policy.durableDelete = false;
record = client.get(policy, key);
exists = client.delete(policy, key);
System.out.println("non-durableDelete: record=" + record + ", exists=" + exists);

policy.durableDelete = false;
record = client.get(policy, key);
exists = client.delete(policy, key);
System.out.println("non-durableDelete2: record=" + record + ", exists=" + exists);

Results:

durableDelete: record=(gen:1),(exp:325379215),(bins:(bin:val)), exists=true
non-durableDelete: record=null, exists=true
non-durableDelete2: record=null, exists=false

If the server returns OK, delete() will return true. If the server returns NOT_FOUND, delete() will return false. If the server returns any other error code, AerospikeException will be thrown.