INTERNAL: Remove redudant operation type RW.

[brido4125]

🔗 Related Issue

https://github.com/jam2in/arcus-works/issues/692

⌨️ What I did

CollectionGet 연산의 경우 dropIfEmpty 또는 withDelete 등의
옵션에 의해 write 연산이 될 가능성을 가진 read 연산이다.

이전에는 이를 구별하기 위해 OperationType.RW를 사용했지만
setAPIReadPriority 메서드에서만 해당 타입이 사용된다.

이 타입을 제거하고 디폴트로는 read를 설정하고
CollectionGetOperationImpl 생성자에서
dropIfEmpty와 withDelete 여부를 확인 한 다음
write 타입으로 변경하도록 한다.
(기존 생성자에서는 dropIfEmpty를 확인하지 않았다)

추가적으로 collectionGet이 read 타입이 디폴트이기 때문에
apiReadPriorityList에 readPriority를 설정할 수 있다.

collectionGet 연산은 op가 생성된 후에 replica pick을 설정하는 api 이기 때문에
write 연산임에도 readPriority가 설정되는 버그가 발생하지는 않는다.

[brido4125]

@uhm0311 @oliviarla

리뷰 노티 한번 더 드립니다

[jhpark816]

리뷰 완료

[jhpark816]

collectionGet.isDropIfEmpty() 조건은 제거합시다.

delete가 false이면 dropIfEmpty가 true이더라도 delete가 수행되지 않습니다.

[oliviarla]

이 pr에서 버그가 있었습니다.

enum 타입은 싱글톤으로 관리되기 때문에 한번 OperationType을 바꾸면 다른 곳에서 객체를 가져올 때 바뀌어진 값을 가져오게 됩니다. 따라서 RW 타입을 그대로 유지하거나 다른 방법을 생각해보아야 합니다.

@Test
void test() {
  ListGet listGet = new ListGet(0, true, true);
  CollectionGetOperationImpl op = new CollectionGetOperationImpl("key", listGet, null);
  APIType type1 = op.getAPIType();
  assertEquals(OperationType.WRITE, type1.getAPIOpType());
  
  ListGet listGet2 = new ListGet(0, false, false);
  CollectionGetOperationImpl op2 = new CollectionGetOperationImpl("key", listGet2, null);
  APIType type2 = op2.getAPIType();
  assertEquals(OperationType.READ, type2.getAPIOpType()); // OperationType.WRITE가 반환되어 테스트가 실패한다.
}

[jhpark816]

@brido4125
위의 코멘트 확인하시고,
기존에 api type과 operation type이 있었습니다.
operation type은 read or write 여부를 확인합니다.
api type은 어떤 용도인가요?

[brido4125]

@jhpark816

api type은 어떤 용도인가요?

현재 구현 상으로는 API Read Priority를 설정하고
이를 사용할 때 활용되는 용도입니다.

아래 PR이 반영될 경우에는 api type을 통해서
opType을 사용하는 용도도 추가됩니다.
#883

@oliviarla
애초에 RW타입이 사용되는 곳이 collectionGet에서만 사용되었습니다.
그래서 RW 타입이 여러 api에서 사용되면 유지시키는게 좋을 것 같은데
그렇지 않으니 차라리 ApiType을 아래와 같이 추가시키는게 좋을 것 같습니다.

  LOP_GET(OperationType.READ),
  LOP_GET_DELETE(OperationType.WRITE),

[jhpark816]

@brido4125 @oliviarla
api type 자체의 용도가 없다는 것이네요.
그러면, operation type을 다시 두고, api type을 제거하는 것이 나을 것 같습니다. 검토해 주세요.

[brido4125]

@jhpark816

그러면 API Type에 따라 read Priority를 설정할 수 있는 기능을 제거하나요?

[brido4125]

@jhpark816 @oliviarla

아래 코멘트처럼 이미 APIType Read Priority 기능이
정식으로 릴리즈 된 기능 입니다.
그래서 갑자기 없애버리거나 deprecated 시키기는 좀 애매하다고 생각됩니다.
https://github.com/jam2in/arcus-works/issues/691#issuecomment-2661856525

그래서 API Type 이넘은 유지를 하고 앞선 코멘트 처럼
구체적으로 API Type을 명시해주는게 좋을 것 같습니다.

[jhpark816]

@brido4125 @oliviarla
api type에서 operation type을 내포하지 않도록 하고,
api type과 operation type을 따로 설정하는 것이 좋을 것 같습니다.

[brido4125]

@jhpark816

살펴보니 APIType이 OperationType을 가지지 않을 경우 아래 문제가 있었네요
얼마전에 고친 ReplicaPick 구현입니다.
Op 인스턴스가 생성되기 전 해다 연산이 Read or Write인지
구별하기 위해 APIType을 사용합니다.

이렇게 될 경우 현재 구현대로 서로 결합된 형태를 가지는게 맞는것 같습니다.

private ReplicaPick getReplicaPick(APIType apiType) {
    ReplicaPick pick = ReplicaPick.MASTER;
    if (apiType.getAPIOpType() == OperationType.READ) {
      ReadPriority readPriority = connFactory.getAPIReadPriority().get(apiType);
      if (readPriority == null) {
        readPriority = connFactory.getReadPriority();
      }
      if (readPriority == ReadPriority.SLAVE) {
        pick = ReplicaPick.SLAVE;
      } else if (readPriority == ReadPriority.RR) {
        pick = ReplicaPick.RR;
      }
    }
    return pick;
  }

그래서 APIType과 OpType이 결합이 되어야한다면
아래 코멘트를 적용시키는게 좋을 것 같습니다.
#879 (comment)

[jhpark816]

Op 인스턴스가 생성되기 전 해다 연산이 Read or Write인지
구별하기 위해 APIType을 사용합니다.

어떤 연산을 생성하는 로직인 지 관련 코드를 제시해 주세요.

[brido4125]

@jhpark816

XopBulkInsert와 같은 groupingKeys를 사용하는 연산들입니다.

[jhpark816]

@brido4125
groupingKeys(), getReplicaPick() 메소드의 인터페이스, 로직을 수정하여
해결하는 방안이 있는 지를 검토해 보시죠.

[brido4125]

@jhpark816

APIType과 OpType을 분리한다는 가정하에서
아래와 같이 인자로 APIType과 OpType을 함께 두는 방안 정도가 가능한 방안인것 같습니다.
(아니면 TypeClass를 둬서 두 가지를 한 객체에 넣어서 사용하는 방안도 존재)

private ReplicaPick getReplicaPick(APIType apiType, OperationType opType) {
    ReplicaPick pick = ReplicaPick.MASTER;
    if (opType == OperationType.READ) {
      ReadPriority readPriority = connFactory.getAPIReadPriority().get(apiType);
      if (readPriority == null) {
        readPriority = connFactory.getReadPriority();
      }
      if (readPriority == ReadPriority.SLAVE) {
        pick = ReplicaPick.SLAVE;
      } else if (readPriority == ReadPriority.RR) {
        pick = ReplicaPick.RR;
      }
    }
    return pick;
  }

특히 groupingKeys 구현이 현재보다 좀 지저분해지는 느낌이 있긴한데,
말씀하신대로 APIType의 가짓수를 늘리는 방안보다는 좋은 구현인 것 같습니다.

[oliviarla]

@jhpark816 @brido4125
api type과 operation type을 따로 설정하게 되면, 강제성이 없어져 getReplicaPick(LOP_INSERT, OperationType.READ) 과 같이 호출이 가능해집니다.

물론 private 메서드이기 때문에 저희가 신경써서 구현하면 되지만, 혼돈을 막고 강제성을 부여하기 위해 ApiType과 OperationType은 결합된 형태로 두는게 나을 것 같습니다.