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

Recover the offset from HDFS, even if topic name is not present in storage path #516

Closed
wants to merge 1 commit into from

Conversation

hariprasad-k
Copy link

@hariprasad-k hariprasad-k commented Sep 9, 2020

#346 Problem
Exactly-once semantics should also work without topic name included in the path.

Solution

The function that recover offsets from file names will use the correct path by taking into account a new configuration flag path_include_topicname introduced with confluentinc/kafka-connect-storage-common#126.

Further, the backward compatibility of appending the topic name is retained by default value true for path_include_topicname.

Does this solution apply anywhere else?
  • yes
  • no
If yes, where?

This change is necessary to retain EOS semantics to support flexible storage partitioning scheme as proposed here: confluentinc/kafka-connect-storage-common#126

Test Strategy

Testing done:
  • Unit tests
  • Integration tests
  • System tests
  • Manual tests

Release Plan

@hariprasad-k hariprasad-k requested a review from a team as a code owner September 9, 2020 13:44
@ghost
Copy link

ghost commented Sep 9, 2020

@confluentinc It looks like @hariprasad-k just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@hariprasad-k
Copy link
Author

Fixes #515

@@ -101,7 +102,12 @@ public static String committedFileName(
}

public static String topicDirectory(String url, String topicsDir, String topic) {
return url + "/" + topicsDir + "/" + topic;
boolean topicInPath = Boolean.valueOf(StorageCommonConfig.PATH_INCLUDE_TOPICNAME_CONFIG);

Choose a reason for hiding this comment

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

This will always be false since you're parsing the config string, not looking up the value in the config

@OneCricketeer
Copy link

Could you please add unit tests?

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