Skip to content

Refactor: Clean up compaction config classes#16810

Merged
kfaraz merged 5 commits intoapache:masterfrom
kfaraz:compaction_audit_entry
Jul 30, 2024
Merged

Refactor: Clean up compaction config classes#16810
kfaraz merged 5 commits intoapache:masterfrom
kfaraz:compaction_audit_entry

Conversation

@kfaraz
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz commented Jul 28, 2024 •
edited
Loading

Changes

  • Rename CoordinatorCompactionConfig to DruidCompactionConfig
  • Rename CompactionConfigUpdateRequest to ClusterCompactionConfig
  • Refactor methods in DruidCompactionConfig
  • Clean up DataSourceCompactionConfigHistory and its tests
  • Clean up tests and add new tests
  • Change API path /druid/coordinator/v1/config/global to /druid/coordinator/v1/config/cluster

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 28, 2024
View reviewed changes
Comment thread ...rc/test/java/org/apache/druid/tests/coordinator/duty/ITAutoCompactionLockContentionTest.java
Comment on lines +306 to +310
final DataSourceCompactionConfig dataSourceCompactionConfig = DataSourceCompactionConfig
.builder()
.forDataSource(fullDatasourceName)
.withSkipOffsetFromLatest(Period.ZERO)
.withMaxRowsPerSegment(Specs.MAX_ROWS_PER_SEGMENT)

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation

Invoking [Builder.withMaxRowsPerSegment](1) should be avoided because it has been deprecated.
@kfaraz kfaraz mentioned this pull request Jul 29, 2024
Merged
10 tasks
gargvishesh
gargvishesh reviewed Jul 29, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@gargvishesh gargvishesh left a comment

Choose a reason for hiding this comment

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

Thanks @kfaraz for the cleanup. Minor comments.

Comment thread ...tests/src/test/java/org/apache/druid/tests/coordinator/duty/ITAutoCompactionUpgradeTest.java
.forDataSource(UPGRADE_DATASOURCE_NAME)
.withSkipOffsetFromLatest(newSkipOffset)
.withTuningConfig(
new UserCompactionTaskQueryTuningConfig(
Copy link
Copy Markdown
Contributor

@gargvishesh gargvishesh Jul 29, 2024

Choose a reason for hiding this comment

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

A builder can be introduced for this one as well in future.

Copy link
Copy Markdown
Contributor Author

@kfaraz kfaraz Jul 30, 2024

Choose a reason for hiding this comment

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

Yeah, it's already in the works. 😄

Comment thread .../src/main/java/org/apache/druid/server/coordinator/DataSourceCompactionConfigAuditEntry.java
@JsonCreator
public DataSourceCompactionConfigAuditEntry(
@JsonProperty("globalConfig") GlobalCompactionConfig globalConfig,
@JsonProperty("globalConfig") ClusterCompactionConfig globalConfig,
Copy link
Copy Markdown
Contributor

@gargvishesh gargvishesh Jul 29, 2024

Choose a reason for hiding this comment

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

Suggested change
@JsonProperty("globalConfig") ClusterCompactionConfig globalConfig,
@JsonProperty("globalConfig") ClusterCompactionConfig clusterCompactionConfig,

Comment thread .../src/main/java/org/apache/druid/server/coordinator/DataSourceCompactionConfigAuditEntry.java

@JsonProperty
public GlobalCompactionConfig getGlobalConfig()
public ClusterCompactionConfig getGlobalConfig()
Copy link
Copy Markdown
Contributor

@gargvishesh gargvishesh Jul 29, 2024

Choose a reason for hiding this comment

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

Suggested change
public ClusterCompactionConfig getGlobalConfig()
public ClusterCompactionConfig getClusterCompactionConfig()

Copy link
Copy Markdown
Contributor Author

@kfaraz kfaraz Jul 30, 2024

Choose a reason for hiding this comment

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

globalConfig is also a term used by the web-console.
Renaming this getter would require us to use @JsonProperty("globalConfig") on the getter for backwards compatibility. Leaving the names as is seemed cleaner.

Let me know if you feel strongly about this.

Comment thread .../src/main/java/org/apache/druid/server/coordinator/DataSourceCompactionConfigAuditEntry.java
public class DataSourceCompactionConfigAuditEntry
{
private final GlobalCompactionConfig globalConfig;
private final ClusterCompactionConfig globalConfig;
Copy link
Copy Markdown
Contributor

@gargvishesh gargvishesh Jul 29, 2024

Choose a reason for hiding this comment

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

Consistent naming may be better. Can use older annotation for getter property name.

Suggested change
private final ClusterCompactionConfig globalConfig;
private final ClusterCompactionConfig clusterCompactionConfig;

Comment thread server/src/main/java/org/apache/druid/server/coordinator/DruidCompactionConfig.java
);
}

public Optional<DataSourceCompactionConfig> findConfigForDatasource(String dataSource)
Copy link
Copy Markdown
Contributor

@gargvishesh gargvishesh Jul 29, 2024

Choose a reason for hiding this comment

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

Thanks for this handy function!

Comment thread server/src/main/java/org/apache/druid/server/coordinator/DruidCompactionConfig.java Outdated
AmatyaAvadhanula
AmatyaAvadhanula reviewed Jul 30, 2024
View reviewed changes
Comment thread server/src/main/java/org/apache/druid/server/coordinator/ClusterCompactionConfig.java
@JsonProperty("compactionEngine") @Nullable CompactionEngine compactionEngine
@JsonProperty("engine") @Nullable CompactionEngine engine
)
{
Copy link
Copy Markdown
Contributor

@AmatyaAvadhanula AmatyaAvadhanula Jul 30, 2024

Choose a reason for hiding this comment

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

I think the constructor should take in null values but create a compaction config with the non-null default fields.
Otherwise the comparison in the Audit history can be incorrect when there are null values but the default values change

Copy link
Copy Markdown
Contributor Author

@kfaraz kfaraz Jul 30, 2024

Choose a reason for hiding this comment

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

I considered that. But that becomes problematic since this payload is used to update cluster configs.
If we assign non-null values in the constructor, it would seem that the values are being updated even if we don't want them to be updated.

The audit entry thing is not an issue as there we always use non-null values.
See DruidCompactionConfig.clusterConfig() method which always returns non-null values.

Copy link
Copy Markdown
Contributor

@AmatyaAvadhanula AmatyaAvadhanula Jul 30, 2024

Choose a reason for hiding this comment

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

Thanks for the clarification!

AmatyaAvadhanula
AmatyaAvadhanula approved these changes Jul 30, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@AmatyaAvadhanula AmatyaAvadhanula left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @kfaraz

@kfaraz kfaraz merged commit 954aaaf into apache:master Jul 30, 2024
@kfaraz kfaraz deleted the compaction_audit_entry branch July 30, 2024 06:47
@kfaraz
Copy link
Copy Markdown
Contributor Author

kfaraz commented Jul 30, 2024

Thanks a lot for the prompt reviews, @AmatyaAvadhanula , @gargvishesh !!

sreemanamala pushed a commit to sreemanamala/druid that referenced this pull request Aug 6, 2024
@kfaraz @sreemanamala
Refactor: Clean up compaction config classes (apache#16810)
ec147e0 
Changes:
- Rename `CoordinatorCompactionConfig` to `DruidCompactionConfig`
- Rename `CompactionConfigUpdateRequest` to `ClusterCompactionConfig`
- Refactor methods in `DruidCompactionConfig`
- Clean up `DataSourceCompactionConfigHistory` and its tests
- Clean up tests and add new tests
- Change API path `/druid/coordinator/v1/config/global` to `/druid/coordinator/v1/config/cluster`
@kfaraz kfaraz added this to the 31.0.0 milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AmatyaAvadhanula AmatyaAvadhanula AmatyaAvadhanula approved these changes

+1 more reviewer

@gargvishesh gargvishesh gargvishesh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

31.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@kfaraz @gargvishesh @github-advanced-security @AmatyaAvadhanula