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

Inconsistent property names for "druid.metadata.storage.tables.xxx" #1469

Merged
merged 1 commit into from
Jul 17, 2015

Conversation

guobingkun
Copy link
Contributor

druid.metadata.storage.tables.segmentTable, druid.metadata.storage.tables.ruleTable and druid.metadata.storage.tables.configTable are actually ignored by Druid, because the JsonProperty specified in MetadataStorageTablesConfig are different from those.

This PR updates the document and adds a serde test.

@himanshug
Copy link
Contributor

I think the question here is whether to correct the documentation or the code. this PR updates the doc with corrected names (which are consistent with other table names) and that seems fine to me

config.getLockTable(MetadataStorageTablesConfig.TASK_ENTRY_TYPE)
);

Assert.assertEquals(
Copy link
Contributor

Choose a reason for hiding this comment

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

this check is very fragile, individual asserts above should be good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@nishantmonu51
Copy link
Member

+1 to correct the docs instead of modifying the code.

@himanshug
Copy link
Contributor

LGTM

but, here is a caveat: if someone used something like
druid.db.tables.configTable=ABC
in their old properties file, which wouldn't have taken any effect due to the code bug. However, if they run ConvertProperties tool, that will be converted to correct property and suddenly ABC will become the config table for them instead of druid_config. This scenario is highly improbable, so I think its fine.

@xvrl @drcrallen what do you think?

@fjy fjy added this to the 0.8.1 milestone Jul 9, 2015
@fjy
Copy link
Contributor

fjy commented Jul 17, 2015

I think it is fine to merge this fix and make a note of it in the forums as well as documenting it. I dont know of any installation that actually overrides this configs, but it may be good to ask

fjy added a commit that referenced this pull request Jul 17, 2015
Inconsistent property names for "druid.metadata.storage.tables.xxx"
@fjy fjy merged commit e21195f into apache:master Jul 17, 2015
@guobingkun guobingkun deleted the table_config branch July 17, 2015 15:20
@guobingkun
Copy link
Contributor Author

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.

None yet

4 participants