Skip to content
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.

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

KafkaSinkTaskPut method improvement, pasrsingMessageKeyfield type specified as Array. #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

esperar
Copy link

@esperar esperar commented Nov 13, 2023

Type Of PR

Improving readability of KafkaSinkTask class code

Change

  1. b809f2c - In the put method of the KafkaSinkTask class, I made modifications to improve the readability of the validation part. Previously, there were two if statements that excluded the same records based on the condition of samplingEnabled being true or false. I modified it to perform the validation in a single iteration.
@Override
    public void put(Collection<SinkRecord> records) {
        for (SinkRecord record : records) {
            if (record.value() != null) {
                try {
                    String value = record.value().toString();
                    if (!samplingEnabled && isFilteringMatch(value)) { 
                        sendRecord(value);
                    } else if (samplingEnabled) {
                        // The shouldSendRecord method handles the validation for samplingPercentage and isFilteringMatch
                        if (shouldSendRecord(value)) {
                            sendRecord(value);
                        }
                    }
                } catch (Exception e) {
                    logError(e);
                }
            }
        }
    }

  1. b809f2c - Added Methods
private boolean shouldSendRecord(String value) {
      return Math.random() < samplingPercentage && isFilteringMatch(value);
}

private void logError(Exception e) {
      log.error(e.getMessage() + " / " + connectorName, e);
}

I created these two private methods to be used in the relevant class.


  1. 8306cf5 - In this change, I noticed that the fields were wrapped with asList, but since it seems to serve the purpose of only iterating, there doesn't seem to be a reason to wrap them as a List. I modified it to an array.
String[] fields = keyParsingField.split(",");

In Korean

한국어 버전

Type Of PR

KafkaSinkTask 클래스 코드의 가독성 향상

Change

  1. b809f2c - KafkaSinkTaskPut 클래스의 put 메서드를 보시면 samplingEnabled를 검사 후 true, false인 경우 두 가지에 if문에서 같은 records를 순외하며 조건을 보고 sendRecord를 할지 말지 정했는데 이러한 검증 부분에 대한 가독성을 향상시키기 위해 한 순회에서 검증을 마치도록 수정했습니다.
@Override
    public void put(Collection<SinkRecord> records) {
        for (SinkRecord record : records) {
            if (record.value() != null) {
                try {
                    String value = record.value().toString();
                    if (!samplingEnabled && isFilteringMatch(value)) { 
                        sendRecord(value);
                    } else if (samplingEnabled) {
                        // shouldSendRecord에서 sampleingPercentage와 isFilteringMatch에 대한 검증 작업을 처리합니다
                        if (shouldSendRecord(value)) {
                            sendRecord(value);
                        }
                    }
                } catch (Exception e) {
                    logError(e);
                }
            }
        }
    }

  1. b809f2c - Add Method
private boolean shouldSendRecord(String value) {
      return Math.random() < samplingPercentage && isFilteringMatch(value);
}

private void logError(Exception e) {
      log.error(e.getMessage() + " / " + connectorName, e);
}

다음과 같은 두 가지의 private 메서드를 만들었습니다. 관련 클래스에서 활용하도록 수정했습니다.


  1. 8306cf5 - 여기 보시면 asList로 fields들을 wrapping하고있는데 오직 순회만 할 목적이라면 List로 wrapping 하는 이유가 없어보입니다. array로 수정했습니다.
String[] fields = keyParsingField.split(",");

@CLAassistant
Copy link

CLAassistant commented Nov 13, 2023

CLA assistant check
All committers have signed the CLA.

@AndersonChoi AndersonChoi self-requested a review November 13, 2023 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants