Bspark/bulk bop insert

[computerphilosopher]

@minkikim89

bop insert를 로깅하는 것이 가장 중요한 일 같아서 일단 커밋을 두 개로 나누었습니다. 첫 번째 커밋은 bop insert 시에만 로깅이 실행되도록 하였고 두 번째 커밋은 모든 콜렉션, 키밸류 관련 명령에서 실행되도록 고쳤습니다.

리뷰가 끝나면 rebase 하도록 하겠습니다.

변경 개요

cmd_in_second.c

cmd_in_second <command> <count>
ex) cmd_in_second bop insert 10000

사내 세미나에서 발표한 기능입니다.

• 메모리 할당을 시작 부분에서만 받도록 해서 오버헤드를 줄였습니다.
• 파일 출력을 별도 쓰레드로 분리시켰습니다.
• write 시스템콜이 마지막 한번만 실행되도록 했습니다.
• 시간을 매번 재지 않고 로그 500개를 묶어서 재도록 바꾸었습니다.
• 별도 lock을 잡지 않고 STATS_LOCK()을 잡은 상태에서 실행되도록 했습니다.

stats.c

• stats_prefix_record 함수의 인자로 client_ip를 넘기도록 바꾸었습니다.

[jhpark816]

@minkikim89
code 기능의 정확성 위주로 먼저 review 해 주시고, coding style 등은 나중에 review 합시다.

[jhpark816]

@computerphilosopher
stats_prefix 정보를 갱신하는 부분에서 logging하자고 하여 진행된 상태입니다.
arcus-memcached 코드를 보면서 logging에 더 적합한 위치나 방법이 있던가요 ?

[computerphilosopher]

stats_prefix 외의 위치라면 process_command()에서 로깅 함수를 호출하는 것이 제일 적절하다고 생각합니다.

명령어를 파싱하는 함수에서 바로 로깅하는 것이기 때문에 흐름상 자연스럽고, 로깅에 필요한 명령어, client ip, key를 모두 인자로 받기 때문입니다.

stats_prefix 에서 로깅할 경우 파싱이 끝난 상태여서 잘못된 명령어가 기록될 가능성은 걱정하지 않아도 되는데 process_command() 로 위치를 바꿀 경우 별도의 예외 처리를 해야할 것 같습니다.

[jhpark816]

@computerphilosopher
본 기능의 구현에 있어, 만족해야 할 사항이 있습니다.

• stats_lock을 추가로 잡지 않는다.
  즉, stats_lock을 잡게 되는 코드에 logging 기능 추가

위 요구 사항을 만족시키면서 적절한 위치와 방안이 있다면,
간단한 pseudo code로 나타내 주세요.
stats_prefix_xxx() 내부에서 logging하는 방법과 비교해 보고자 합니다.

[computerphilosopher]

#memcached.c
def process_command():
    if cmd_in_second.state == on_logging:
    	STATS_LOCK()
	cmd_in_second.write(command, key, client_ip)
    	STATS_UNLOCK()
    
    if command == "get" or command == "bget":
    	process_get_command(command)
    ...
    elif command == "cmd_in_second" and cmd_in_second.state == not_started:
    	process_second_command(command)
    else:
    	print("ERROOR unkown command")
        return

def process_second_command(command):
    if bad_command_line_format(command):
        return False
    
    if already_started:
    	return False
        
    cmd_in_second.start()
    return True

#cmd_in_second.c
def start(command):
    if unknown_command(command):
    	return False
    
    buffer.init()
    timer.init()
    
    return True

def write(command, key, client_ip):
    if bad_command_line_format(command):
        return False
    
    if not command_to_log(command):
    	return False
    
    log = (command, key, client_ip)
    buffer.add(log)
    
    if buffer.full() and timer.diff(buffer) <= 1
    	buffer.flush()

    return True

[jhpark816]

@computerphilosopher @minkikim89
코드를 확인해 보니, btree_elem_insert() 호출 직후가 가장 적합합니다.

• 기존 stats 정보들도 모두 해당 engine 함수 호출 후에 갱신합니다.
• logging 대상 요청인지 판별하여 logging 작업을 수행합니다.
• stats_prefix_xxx() 함수 내부가 적당하긴 하나, 아래 이슈가 있습니다.
  • connection 구조체 포인터가 인자로 추가되어야 하고,
  • prefix stats을 다루는 곳에서 logging하는 것이 어색한 감이 있네요.
• 따라서, stats_prefix_xxx() 수행 이후에
  • 조건 검사 후, command logging 함수를 호출하고,
  • command logging 내부에 별도 lock을 두어 사용해도 될 것 같습니다.

[minkikim89]

@computerphilosopher
확인해보고 의견주세요.

[computerphilosopher]

@minkikim89

리뷰 사항 반영해서 다시 푸쉬하였습니다.

[computerphilosopher]

@minkikim89

STATS_LOCK 대신 별도 락을 가지는 버전을 푸쉬했습니다.

[minkikim89]

간단하게 한번 리뷰했습니다.

[minkikim89]

아직 코드 리뷰가 많이 필요하겠네요.

[minkikim89]

다른 코드와 되도록 비슷한 형식으로 짜주어야 코드 보는 사람이 편합니다. 다른 쪽에서 다 파라미터 이름을 logger로 하고 있기 때문에 여기서는 logger 라는 파라미터 이름을 써주세요.
여기서 한번에 다 말하자면, 다른 변수명들이나 코드 스타일도 마찬가지에요. 다른쪽을 참고하여 최대한 동일하게 가져가려는 노력이 필요해요. 예를들어 현재 cmd_in_second 구조체 변수를 this라는 이름으로 사용하고 있는데 c언어에서 잘 사용하지 않은 형식의 변수명입니다. 다른 코드들을 참고하여 전체적으로 일반적으로 쓰이는 변수명을 택했으면 좋겠어요.

[minkikim89]

thread 관련 변수를 사용하는 경우가 모든 함수에서 그렇게 많지 않은데 매번 포인터를 정의해서 처리하고 있어요. 이런식으로 하지말고 this.flusher.attr 이런식으로 바로 쓰는게 오해의 여지가 적을 것 같아요.
또 const 키워드를 의식적으로 계속 사용하고 있는데, 필요없는 상황에서 사용한 경우가 대다수에요. 다른 코드들에서 const 키워드가 어떤식으로 사용되는지를 확인하고 사용해주세요. 만약에 다른쪽을 보고서도 어떤 상황에서 쓰이는지 모르겠으면 물어봐요.

[minkikim89]

파일 오픈도 start 시에 하도록 하는게 좋겠네요.

[minkikim89]

cmd_in_second stop 기능도 있어야겠어요.

[minkikim89]

지금하는 방식으로 buffer에 log 삽입하면 데이터 복제가 추가적으로 일어나요.
위에서 log 선언해서 로컬 변수로 log 구조체를 만들었는데 해당 구조체를 buffer에 삽입할 때 다시 한번 복제하고 있어요. 이렇게 log 구조체를 미리 만들고 그것을 복사하여 버퍼에 넣는 것보다는, 이미 buffer에 공간이 있기 때문에 그 공간만을 가져와서 해당 공간에다가 바로 데이터를 복제하는 편이 맞아요.

[jhpark816]

@computerphilosopher
PR에 이슈 링크를 추가해 주세요.

[computerphilosopher]

@jhpark816

관련 이슈가 아직 안 만들어졌습니다. 이 저장소에 새로 하나 만들까요?

[jhpark816]

@computerphilosopher
이 내용에 관한 이슈는 arcus-works에 만들어 주세요.

[computerphilosopher]

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