Skip to content

FEATURE: Replace BopGetArgs with operation-specific args classes #1094

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.

Sign up for GitHub

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

Open
f1v3-dev wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

FEATURE: Replace BopGetArgs with operation-specific args classes #1094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 22 additions & 29 deletions src/main/java/net/spy/memcached/v2/AsyncArcusCommands.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@
import net.spy.memcached.v2.vo.BTreeUpdateElement;
import net.spy.memcached.v2.vo.BopDeleteArgs;
import net.spy.memcached.v2.vo.BopGetArgs;
import net.spy.memcached.v2.vo.BopRangeGetArgs;
import net.spy.memcached.v2.vo.BopSMGetArgs;
import net.spy.memcached.v2.vo.GetOption;

public class AsyncArcusCommands<T> implements AsyncArcusCommandsIF<T> {
Expand Down Expand Up @@ -1353,7 +1355,8 @@ public void gotData(String bKey, int flags, byte[] data, byte[] eFlag) {
}

@Override
public ArcusFuture<BTreeGetResult<T>> bopGet(String key, BKey from, BKey to, BopGetArgs args) {
public ArcusFuture<BTreeGetResult<T>> bopGet(String key, BKey from, BKey to,
BopRangeGetArgs args) {
verifyBKeyTypesMatch(from, to);

AbstractArcusResult<BTreeGetResult<T>> result =
Expand Down Expand Up @@ -1405,34 +1408,33 @@ public void gotData(String bKey, int flags, byte[] data, byte[] eFlag) {

private static BTreeGet createBTreeGet(BKey bKey, BopGetArgs args) {
if (bKey.getType() == BKey.BKeyType.LONG) {
return new BTreeGet((long) bKey.getData(), args.getElementFlagFilter(),
return new BTreeGet((long) bKey.getData(), args.getEFlagFilter(),
args.isWithDelete(), args.isDropIfEmpty());
}

return new BTreeGet((byte[]) bKey.getData(), args.getElementFlagFilter(),
return new BTreeGet((byte[]) bKey.getData(), args.getEFlagFilter(),
args.isWithDelete(), args.isDropIfEmpty());
}

private static BTreeGet createBTreeGet(BKey from, BKey to, BopGetArgs args) {
private static BTreeGet createBTreeGet(BKey from, BKey to, BopRangeGetArgs args) {
if (from.getType() == BKey.BKeyType.LONG) {
return new BTreeGet((long) from.getData(), (long) to.getData(),
args.getElementFlagFilter(), args.getOffset(), args.getCount(),
args.getEFlagFilter(), args.getOffset(), args.getCount(),
args.isWithDelete(), args.isDropIfEmpty());
}

return new BTreeGet((byte[]) from.getData(), (byte[]) to.getData(),
args.getElementFlagFilter(), args.getOffset(), args.getCount(),
args.getEFlagFilter(), args.getOffset(), args.getCount(),
args.isWithDelete(), args.isDropIfEmpty());
}

@Override
public ArcusFuture<Map<String, BTreeGetResult<T>>> bopMultiGet(List<String> keys,
BKey from, BKey to,
BopGetArgs args) {
BopRangeGetArgs args) {
keyValidator.validateKey(keys);
keyValidator.checkDupKey(keys);
verifyBKeyTypesMatch(from, to);
verifyPositiveCountArg(args, ArcusClient.MAX_GETBULK_ELEMENT_COUNT);

ArcusClient client = arcusClientSupplier.get();
Collection<Map.Entry<MemcachedNode, List<String>>> arrangedKeys =
Expand Down Expand Up @@ -1542,25 +1544,24 @@ public void gotElement(String key, int flags, Object bKey, byte[] eFlag, byte[]
}

private BTreeGetBulk<T> createBTreeGetBulk(MemcachedNode node, List<String> keys,
BKey from, BKey to, BopGetArgs args) {
BKey from, BKey to, BopRangeGetArgs args) {
if (from.getType() == BKey.BKeyType.LONG) {
return new BTreeGetBulkWithLongTypeBkey<>(node, keys,
(long) from.getData(), (long) to.getData(), args.getElementFlagFilter(),
(long) from.getData(), (long) to.getData(), args.getEFlagFilter(),
args.getOffset(), args.getCount());
}

return new BTreeGetBulkWithByteTypeBkey<>(node, keys,
(byte[]) from.getData(), (byte[]) to.getData(), args.getElementFlagFilter(),
(byte[]) from.getData(), (byte[]) to.getData(), args.getEFlagFilter(),
args.getOffset(), args.getCount());
}

@Override
public ArcusFuture<BTreeSMGetResult<T>> bopSortMergeGet(List<String> keys, BKey from, BKey to,
boolean unique, BopGetArgs args) {
BopSMGetArgs args) {
keyValidator.validateKey(keys);
keyValidator.checkDupKey(keys);
verifyBKeyTypesMatch(from, to);
verifyPositiveCountArg(args, ArcusClient.MAX_SMGET_COUNT);

ArcusClient client = arcusClientSupplier.get();
Collection<Map.Entry<MemcachedNode, List<String>>> arrangedKeys =
Expand All @@ -1569,7 +1570,7 @@ public ArcusFuture<BTreeSMGetResult<T>> bopSortMergeGet(List<String> keys, BKey
List<CompletableFuture<BTreeSMGetResult<T>>> smGetFutures = new ArrayList<>();

for (Map.Entry<MemcachedNode, List<String>> entry : arrangedKeys) {
BTreeSMGet<T> smGet = createBTreeSMGet(from, to, args, unique, entry);
BTreeSMGet<T> smGet = createBTreeSMGet(from, to, args, entry);
CompletableFuture<BTreeSMGetResult<T>> future =
bopSortMergeGetPerNode(client, smGet).toCompletableFuture();
smGetFutures.add(future);
Expand All @@ -1585,8 +1586,8 @@ public ArcusFuture<BTreeSMGetResult<T>> bopSortMergeGet(List<String> keys, BKey
results.add(future.join());
}
}
return BTreeSMGetResult.mergeSMGetElements(results, from.compareTo(to) <= 0, unique,
args.getCount());
return BTreeSMGetResult.mergeSMGetElements(results, from.compareTo(to) <= 0,
args.isUnique(), args.getCount());
});
}

Expand Down Expand Up @@ -1656,18 +1657,17 @@ public void gotTrimmedKey(String key, Object bKey) {
return future;
}

private BTreeSMGet<T> createBTreeSMGet(BKey from, BKey to, BopGetArgs args,
boolean unique,
private BTreeSMGet<T> createBTreeSMGet(BKey from, BKey to, BopSMGetArgs args,
Map.Entry<MemcachedNode, List<String>> entry) {
if (from.getType() == BKey.BKeyType.LONG) {
return new BTreeSMGetWithLongTypeBkey<>(entry.getKey(), entry.getValue(),
(long) from.getData(), (long) to.getData(), args.getElementFlagFilter(),
args.getCount(), unique);
(long) from.getData(), (long) to.getData(),
args.getEFlagFilter(), args.getCount(), args.isUnique());
}

return new BTreeSMGetWithByteTypeBkey<>(entry.getKey(), entry.getValue(),
(byte[]) from.getData(), (byte[]) to.getData(), args.getElementFlagFilter(),
args.getCount(), unique);
(byte[]) from.getData(), (byte[]) to.getData(),
args.getEFlagFilter(), args.getCount(), args.isUnique());
}

@Override
Expand Down Expand Up @@ -1933,13 +1933,6 @@ private static void verifyBKeyTypesMatch(BKey from, BKey to) {
}
}

private static void verifyPositiveCountArg(BopGetArgs args, int maxCount) {
int count = args.getCount();
if (count <= 0 || count > maxCount) {
throw new IllegalArgumentException("Count should be between 1 to " + maxCount);
}
}

private ArcusFuture<Boolean> collectionCreate(String key, CollectionCreate collectionCreate) {
AbstractArcusResult<Boolean> result = new AbstractArcusResult<>(new AtomicReference<>());
ArcusFutureImpl<Boolean> future = new ArcusFutureImpl<>(result);
Expand Down
11 changes: 6 additions & 5 deletions src/main/java/net/spy/memcached/v2/AsyncArcusCommandsIF.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import net.spy.memcached.v2.vo.BTreeUpdateElement;
import net.spy.memcached.v2.vo.BopDeleteArgs;
import net.spy.memcached.v2.vo.BopGetArgs;
import net.spy.memcached.v2.vo.BopRangeGetArgs;
import net.spy.memcached.v2.vo.BopSMGetArgs;
import net.spy.memcached.v2.vo.GetOption;

public interface AsyncArcusCommandsIF<T> {
Expand Down Expand Up @@ -694,7 +696,7 @@ ArcusFuture<Map.Entry<Boolean, BTreeElement<T>>> bopUpsertAndGetTrimmed(
* empty {@code BTreeGetResult} if no elements are found in the range but key exists,
* {@code null} if key is not found
*/
ArcusFuture<BTreeGetResult<T>> bopGet(String key, BKey from, BKey to, BopGetArgs args);
ArcusFuture<BTreeGetResult<T>> bopGet(String key, BKey from, BKey to, BopRangeGetArgs args);

/**
* Get elements from multiple btree items.
Expand All @@ -708,21 +710,20 @@ ArcusFuture<Map.Entry<Boolean, BTreeElement<T>>> bopUpsertAndGetTrimmed(
* no {@code Map.Entry} in the map if the key is not found
*/
ArcusFuture<Map<String, BTreeGetResult<T>>> bopMultiGet(
List<String> keys, BKey from, BKey to, BopGetArgs args);
List<String> keys, BKey from, BKey to, BopRangeGetArgs args);

/**
* Get sort-merged elements from multiple btree items.
*
* @param keys list of keys to get
* @param from BKey range start
* @param to BKey range end
* @param unique whether to return unique elements only
* @param args arguments for get operation
* @param args arguments for get operation {@link BopSMGetArgs}
* @return {@code BTreeSMGetResult} containing sort-merged elements,
* empty {@code BTreeSMGetResult} if no matching elements exist
*/
ArcusFuture<BTreeSMGetResult<T>> bopSortMergeGet(
List<String> keys, BKey from, BKey to, boolean unique, BopGetArgs args);
List<String> keys, BKey from, BKey to, BopSMGetArgs args);

/**
* Get the position of an element with the given bKey in a btree item.
Expand Down
91 changes: 12 additions & 79 deletions src/main/java/net/spy/memcached/v2/vo/BopGetArgs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,97 +4,30 @@

public final class BopGetArgs {

public static final BopGetArgs DEFAULT = new BopGetArgs.Builder().build();
public static final BopGetArgs DEFAULT
= new BopGetArgs(ElementFlagFilter.DO_NOT_FILTER, GetOption.NONE);

private final ElementFlagFilter eFlagFilter;
private final int offset;
private final int count;
private final boolean withDelete;
private final boolean dropIfEmpty;
private final GetOption option;

public BopGetArgs(ElementFlagFilter eFlagFilter, GetOption option) {
if (option == null) {
throw new IllegalArgumentException("option cannot be null");
}

private BopGetArgs(ElementFlagFilter eFlagFilter, int offset, int count,
boolean withDelete, boolean dropIfEmpty) {
this.eFlagFilter = eFlagFilter;
this.offset = offset;
this.count = count;
this.withDelete = withDelete;
this.dropIfEmpty = dropIfEmpty;
this.option = option;
}

public ElementFlagFilter getElementFlagFilter() {
public ElementFlagFilter getEFlagFilter() {
return eFlagFilter;
}

public int getOffset() {
return offset;
}

public int getCount() {
return count;
}

public boolean isWithDelete() {
return withDelete;
return option.isWithDelete();
}

public boolean isDropIfEmpty() {
return dropIfEmpty;
}

public static final class Builder {
private ElementFlagFilter eFlagFilter = null;
private int offset = 0;
private int count = 50;
private boolean withDelete = false;
private boolean dropIfEmpty = false;

public Builder eFlagFilter(ElementFlagFilter eFlagFilter) {
this.eFlagFilter = eFlagFilter;
return this;
}

/**
* Set the offset only for {@code AsyncArcusCommands#bopGet}
* or {@code AsyncArcusCommands#bopMultiGet}
*
* @param offset to skip elements that match condition from the 'from' BKey
*/
public Builder offset(int offset) {
if (offset < 0) {
throw new IllegalArgumentException("offset cannot be negative");
}
this.offset = offset;
return this;
}

/**
* Set the count of elements to retrieve.
*
* @param count For bopGet or bopMultiGet method,
* set the number of elements to retrieve from each BTree item.
* For bopSortMergeGet method,
* set the total number of elements to retrieve across all BTree items.
*/
public Builder count(int count) {
if (count < 0) {
throw new IllegalArgumentException("count cannot be negative");
}
this.count = count;
return this;
}

public Builder withDelete() {
this.withDelete = true;
return this;
}

public Builder dropIfEmpty() {
this.dropIfEmpty = true;
return this;
}

public BopGetArgs build() {
return new BopGetArgs(eFlagFilter, offset, count, withDelete, dropIfEmpty);
}
return option.isDropIfEmpty();
}
}
53 changes: 53 additions & 0 deletions src/main/java/net/spy/memcached/v2/vo/BopRangeGetArgs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package net.spy.memcached.v2.vo;

import net.spy.memcached.collection.ElementFlagFilter;

public class BopRangeGetArgs {

public static final BopRangeGetArgs DEFAULT
= new BopRangeGetArgs(ElementFlagFilter.DO_NOT_FILTER, 0, 50, GetOption.NONE);
Copy link
Copy Markdown
Collaborator

@jhpark816 jhpark816 May 21, 2026

Choose a reason for hiding this comment

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

테스트 목적으로만 사용될 것으로 보이며,
테스트 쪽 코드에 두는 것은 어떤지?

Copy link
Copy Markdown
Collaborator Author

@f1v3-dev f1v3-dev May 21, 2026

Choose a reason for hiding this comment

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

BopGetArgs.DEFAULT는 eFlag filter 없이 단순 조회하는 케이스가 많다고 판단하여
ElementFlagFilter.DO_NOT_FILTER + GetOption.NONE 조합으로 제공했습니다.

이번 PR의 목적이 BopGetArgs를 연산별 args로 분리하는 것이다 보니
같은 취지로 BopRangeGetArgs.DEFAULT, BopSMGetArgs.DEFAULT도 추가를 한 상태입니다.

다만 BopRangeGetArgs와 BopSMGetArgs는 단순 옵션뿐 아니라
offset, count, unique처럼 조회 결과 범위와 동작에 영향을 주는 값을 포함하여 어색해보이기는 하는데 제거하는 방향이 좋을까요?

Copy link
Copy Markdown
Collaborator

@jhpark816 jhpark816 May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

여기도 논의 대상이군요.
각 연산의 semantic에 따라 인자들의 default 값을 정한다면 아래와 같으니, 참고 바랍니다.

  • bop range get에서 offset, count의 default 값은 0이어야 함.
  • bop multi get(mget)에서 offset만 default 값이 0이어야 하고, count 값은 입력 필요.
    • count의 default 값을 50으로 정해도 될 것 같음
  • bop smget에서 unique만 default 값이 false이어야 하고, count 값은 입력 필요

지금 다시 생각해 보면, 아래 값을 넘기는 것 자체도 어색하지 않나 생각됩니다.

  • ElementFlagFilter.DO_NOT_FILTER
  • GetOption.NONE
  • BopGetArgs.DEFAULT

따라서, 아래와 같이 인자들은 모두 풀어서 표현하고(인자 수가 많지 않음),
필수 인자는 항상 값이 주어져야 하고, 나머지는 Nullable 인자로 생략 시 null을 넘기는 것도
방안이라고 생각합니다. 검토 바랍니다.

// 필수 인자: key, bKey
// Nullable 인자 : eFilter, deleteOption
bopGet(key, bKey, eFilter, deleteOption) // 4가지 호출 형태
- bopGet(key, bKey, null, null);
- bopGet(key, bKey, eFilter, null);
- bopGet(key, bKey, null, deleteOption);
- bopGet(key, bKey, eFilter, deleteOption);

// 필수 인자: key, bKey, offset, count
// Nullable 인자 : eFilter, deleteOption
bopGet(key, from, to, eFilter, offset, count, deleteOption) // 4가지 호출 형태
- bopGet(key, from, to, null, offset, count, null)
- bopGet(key, from, to, eFilter, offset, count, null)
- bopGet(key, from, to, null, offset, count, deleteOption)
- bopGet(key, from, to, eFilter, offset, count, deleteOption)

// 필수 인자: key, bKey, offset, count
// Nullable 인자 : eFilter
bopMultiGet(keys, from, to, eFilter, offset, count) // 2가지 호출 형태
- bopMultiGet(keys, from, to, null, offset, count)
- bopMultiGet(keys, from, to, eFilter, offset, count)

// 필수 인자: key, bKey, count, unique
// Nullable 인자 : eFilter
bopSortMergeGet(keys, from, to, eFilter, count, unique) // 2가지 호출 형태;
- bopSortMergeGet(keys, from, to, null, count, unique)
- bopSortMergeGet(keys, from, to, eFilter, count, unique)


private final ElementFlagFilter eFlagFilter;
private final int offset;
private final int count;
private final GetOption option;

public BopRangeGetArgs(ElementFlagFilter eFlagFilter, int offset, int count, GetOption option) {
if (offset < 0) {
throw new IllegalArgumentException("offset cannot be negative");
}

if (count < 1 || count > 50) {
Copy link
Copy Markdown
Collaborator

@jhpark816 jhpark816 May 21, 2026

Choose a reason for hiding this comment

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

50 이라는 제한이 있어야 하나요?

Copy link
Copy Markdown
Collaborator Author

@f1v3-dev f1v3-dev May 21, 2026

Choose a reason for hiding this comment

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

현재 BopRangeGetArgs를 bop get range 조회와 bop mget에서 공통으로 사용하면서 bop mget의 제약 조건인 1 <= count <= 50이 bop get range 조회에도 적용되는 문제로 보입니다.

고민을 해본 결과 아래와 같이 수정할 수 있을 것 같습니다.

  1. BopRangeGetArgs에서는 count >= 1만 검증하고, bopMultiGet() 내부에서만 count <= 50을 검증
  2. BopMGetArgs를 별도로 만들고, bop mget 스펙에 맞게 1 <= count <= 50을 검증

참고로 bop mget이 delete/drop 옵션을 지원하지 않는 점까지 고려하면, BopMGetArgs로 분리하는 방향도 괜찮아 보입니다.

Copy link
Copy Markdown
Collaborator

@jhpark816 jhpark816 May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

논의 대상이군요.
그리고, 아래 조건이어야 함을 참고 바랍니다.

  • bop range get: count >= 0 : 0이어도 됩니다.
  • bop multi get: 1 <= count <= 50

나중에 bop mget에서도 delete/drop 지원할 가능성이 있으므로, 아래와 같이 하는 것이 어떤가 생각합니다.

  • BopRangeGetArgs에서는 count >= 0만 검증하고,
  • bopMultiGet() 내부에서만 1 <= count <= 50 && no delete/drop을 검증

그리고, DEFAULT 값은 offset = 0, count = 0 이어야 합니다.

throw new IllegalArgumentException("count must be between 1 and 50");
}

if (option == null) {
throw new IllegalArgumentException("option cannot be null");
}

this.eFlagFilter = eFlagFilter;
this.offset = offset;
this.count = count;
this.option = option;
}

public ElementFlagFilter getEFlagFilter() {
return eFlagFilter;
}

public int getOffset() {
return offset;
}

public int getCount() {
return count;
}

public boolean isWithDelete() {
return option.isWithDelete();
}

public boolean isDropIfEmpty() {
return option.isDropIfEmpty();
}
}
35 changes: 35 additions & 0 deletions src/main/java/net/spy/memcached/v2/vo/BopSMGetArgs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package net.spy.memcached.v2.vo;

import net.spy.memcached.collection.ElementFlagFilter;

public class BopSMGetArgs {

public static final BopSMGetArgs DEFAULT
= new BopSMGetArgs(ElementFlagFilter.DO_NOT_FILTER, 1000, false);

private final ElementFlagFilter eFlagFilter;
private final int count;
private final boolean unique;

public BopSMGetArgs(ElementFlagFilter eFlagFilter, int count, boolean unique) {
if (count < 1 || count > 1000) {
Copy link
Copy Markdown
Collaborator

@jhpark816 jhpark816 May 21, 2026

Choose a reason for hiding this comment

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

1000 값은 MAX_SMGET_COUNT 형태로 상수화하여 사용하는 것은 어떤지?

Copy link
Copy Markdown
Collaborator Author

@f1v3-dev f1v3-dev May 21, 2026

Choose a reason for hiding this comment

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

1000 이라는 값을 SMGet count 제한이라는 의미를 드러낼 수 있어서 상수화 해도 좋을 것 같습니다.

반영하겠습니다.

throw new IllegalArgumentException("count must be between 1 and 1000");
}

this.eFlagFilter = eFlagFilter;
this.count = count;
this.unique = unique;
}

public ElementFlagFilter getEFlagFilter() {
return eFlagFilter;
}

public int getCount() {
return count;
}

public boolean isUnique() {
return unique;
}
}
Loading
Loading