Skip to content

[HUDI-4830] Fix testNoGlobalConfFileConfigured when add hudi-defaults.conf in default dir #6652

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

Merged
xushiyan merged 4 commits into from
Sep 26, 2022
Merged

[HUDI-4830] Fix testNoGlobalConfFileConfigured when add hudi-defaults.conf in default dir #6652

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b4a114
[HUDI-4830] Fix testNoGlobalConfFileConfigured when add hudi-defaults…
Zouxxyy Sep 11, 2022
5492efb
fix
Zouxxyy Sep 23, 2022
645096c
fix
Zouxxyy Sep 23, 2022
1389d15
Merge branch 'apache:master' into zouxxyy/fix-testNoGlobalConfFileCon…
Zouxxyy Sep 25, 2022
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
4 changes: 3 additions & 1 deletion hudi-common/src/test/java/org/apache/hudi/common/util/TestDFSPropertiesConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,9 @@ public void testNoGlobalConfFileConfigured() {
ENVIRONMENT_VARIABLES.clear(DFSPropertiesConfiguration.CONF_FILE_DIR_ENV_NAME);
// Should not throw any exception when no external configuration file configured
DFSPropertiesConfiguration.refreshGlobalProps();
assertEquals(0, DFSPropertiesConfiguration.getGlobalProps().size());
DFSPropertiesConfiguration defaultDfsPropertiesConfiguration = new DFSPropertiesConfiguration();
defaultDfsPropertiesConfiguration.addPropsFromFile(DFSPropertiesConfiguration.DEFAULT_PATH);
assertEquals(defaultDfsPropertiesConfiguration.getProps().size(), DFSPropertiesConfiguration.getGlobalProps().size());
Copy link
Contributor

@yihua yihua Sep 13, 2022

Choose a reason for hiding this comment

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

Is this test broken in CI? I could not reproduce the failure.

Copy link
Contributor Author

@Zouxxyy Zouxxyy Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

DFSPropertiesConfiguration.refreshGlobalProps() only refresh the system props, and the test will fail when/etc/hudi/conf/hudi-defaults.conf exists locally.

Copy link
Member

@xushiyan xushiyan Sep 16, 2022

Choose a reason for hiding this comment

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

let's not create DFSPropertiesConfiguration again to generate the expected value. we should just load the default hudi-defaults.conf with plain java properties and get the count as the expected size.

Copy link
Member

@xushiyan xushiyan Sep 16, 2022

Choose a reason for hiding this comment

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

so the logic should be count the configs from hudi-defaults.conf at the default path if exists, or make it 0 if not exists. @Zouxxyy can you make change for this please? then we should be good to land

Copy link
Contributor Author

@Zouxxyy Zouxxyy Sep 16, 2022

Choose a reason for hiding this comment

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

so the logic should be count the configs from hudi-defaults.conf at the default path if exists, or make it 0 if not exists. @Zouxxyy can you make change for this please? then we should be good to land

hudi uses it's own method to read props instead of Apache commons PropertiesConfiguration, just like this

  public void addPropsFromStream(BufferedReader reader) throws IOException {
    try {
      reader.lines().forEach(line -> {
        if (!isValidLine(line)) {
          return;
        }
        String[] split = splitProperty(line);
        if (line.startsWith("include=") || line.startsWith("include =")) {
          Path includeFilePath = new Path(currentFilePath.getParent(), split[1]);
          addPropsFromFile(includeFilePath);
        } else {
          hoodieConfig.setValue(split[0], split[1]);
        }
      });

    } finally {
      reader.close();
    }
  }

currently there is no static method to get prop directly from a path

Copy link
Member

@xushiyan xushiyan Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@Zouxxyy then we need a different way to load the default conf file and set it as the expected value. the current way is not the right way for testing: both expected and tested use DFSPropertiesConfiguration to load. what if there is a bug with DFSPropertiesConfiguration loading conf file and both return unexpected numbers, the test would still pass but not catching bug

Copy link
Contributor Author

@Zouxxyy Zouxxyy Sep 23, 2022

Choose a reason for hiding this comment

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

@Zouxxyy then we need a different way to load the default conf file and set it as the expected value. the current way is not the right way for testing: both expected and tested use DFSPropertiesConfiguration to load. what if there is a bug with DFSPropertiesConfiguration loading conf file and both return unexpected numbers, the test would still pass but not catching bug

I made some modifications.
I think this test case is only used to test the effect of clearing env HUDI_CONF_DIR, so the test can be ignored when hudi-defaults.conf exists locally.
And if you want to test loading hudi-defaults.conf, it should be covered by other tests, such as testParsing, testLoadGlobalConfFile, etc.

}

@Test
Expand Down