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

[RoutineLoad] Support modify routine load job #4158

Merged
merged 12 commits into from
Aug 6, 2020

Conversation

morningman
Copy link
Contributor

Proposed changes

Support ALTER ROUTINE LOAD JOB stmt, for example:

alter routine load db1.label1
properties
(
"desired_concurrent_number"="3",
"max_batch_interval" = "5",
"max_batch_rows" = "300000",
"max_batch_size" = "209715200",
"strict_mode" = "false",
"timezone" = "+08:00"
)

Details can be found in alter-routine-load.md

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have create an issue on fix [New Feature] Support modify routine load property #4157, and have described the bug/feature there in detail
  • Compiling and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • If this change need a document change, I have updated the document
  • Any dependent changes have been merged

@morningman morningman added kind/feature Categorizes issue or PR as related to a new feature. area/load Issues or PRs related to all kinds of load labels Jul 23, 2020
@morningman morningman self-assigned this Jul 23, 2020
RoutineLoadJob job = checkPrivAndGetJob(stmt.getDbName(), stmt.getLabel());
if (stmt.hasDataSourceProperty()
&& !stmt.getDataSourceProperties().getType().equalsIgnoreCase(job.dataSourceType.name())) {
throw new DdlException("The spciefied job type is not: " + stmt.getDataSourceProperties().getType());
Copy link
Contributor

Choose a reason for hiding this comment

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

‘spciefied’ Wrong characters

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

| KW_ALTER KW_DATABASE ident:dbName KW_RENAME ident:newDbName
{:
RESULT = new AlterDatabaseRename(dbName, newDbName);
:}
| KW_ALTER KW_ROUTINE KW_LOAD job_label:jobLabel opt_properties:properties
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| KW_ALTER KW_ROUTINE KW_LOAD job_label:jobLabel opt_properties:properties
| KW_ALTER KW_ROUTINE KW_LOAD job_label:jobLabel opt_properties:jobProperties

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

| KW_ALTER KW_DATABASE ident:dbName KW_RENAME ident:newDbName
{:
RESULT = new AlterDatabaseRename(dbName, newDbName);
:}
| KW_ALTER KW_ROUTINE KW_LOAD job_label:jobLabel opt_properties:jobProperties
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe KW_ALTER KW_ROUTINE KW_LOAD KW_FOR job_label:jobLabel is better. The other routine load stmt also has KW_FOR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -1397,4 +1401,36 @@ public void readFields(DataInput in) throws IOException {
throw new IOException("error happens when parsing create routine load stmt: " + origStmt, e);
}
}

abstract public void modifyProperties(AlterRoutineLoadStmt stmt) throws DdlException;
Copy link
Contributor

Choose a reason for hiding this comment

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

The job properties maybe could be modified in here instead of the subclass KafkaRoutineLoad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we could do this when we got second derived class from RoutineLoadJob


abstract public void modifyProperties(AlterRoutineLoadStmt stmt) throws DdlException;

abstract public void replayModifyProperties(AlterRoutineLoadJobOperationLog log);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@@ -409,6 +409,7 @@ module.exports = [
title: "DML",
directoryPath: "Data Manipulation/",
children: [
"alter-routine-load",
Copy link
Contributor

Choose a reason for hiding this comment

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

Put it between pause routine load and stop routine load.

@morningman morningman force-pushed the modify_routine_load branch from a94cb00 to 4b42b7d Compare August 3, 2020 10:50
Copy link
Contributor

@EmmyMiao87 EmmyMiao87 left a comment

Choose a reason for hiding this comment

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

LGTM

@EmmyMiao87 EmmyMiao87 added the approved Indicates a PR has been approved by one committer. label Aug 6, 2020
@morningman morningman merged commit 237c080 into apache:master Aug 6, 2020
@EmmyMiao87 EmmyMiao87 mentioned this pull request Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. area/load Issues or PRs related to all kinds of load kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Feature] Support modify routine load property
4 participants