-
Notifications
You must be signed in to change notification settings - Fork 32
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
[FLINK-30460] Support the writable metadata ttl. #33
Conversation
Please take a look when you are free, I'll always active util this PR finish. CC @ferenc-csaky @MartijnVisser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, added a couple minor comments.
if (row.isNullAt(pos)) { | ||
throw new IllegalArgumentException( | ||
String.format("Writable metadata '%s' can not accept null value", KEY)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this null
check is basically duplicated and probably will be required for any new metadata type we might have in the future. I think we could separate this into a private static
method in WritableMetadata
, something like validateNotNull(RowData row, int pos, String key)
.
return row.getLong(pos); | ||
} | ||
|
||
public static TimeToLiveMetadata of(List<String> metadataKeys, DataType physicalDataType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I am not against static factory functions at all but in this particular case we could simply have one constructor that takes these 2 args, e.g.:
public TimeToLiveMetadata(List<String> metadataKeys, DataType physicalDataType) {
int idx = metadataKeys.indexOf(KEY);
pos = idx < 0 ? -1 : idx + physicalDataType.getLogicalType().getChildren().size();
}
Because at the moment the TimeToLiveMetadata(int)
ctor is only used in the static factory, so we could spare some code. The same is true for TimestampMetadata
.
But feel free to ignore this comment if you prefer it this way.
TableResult firstResult = tEnv.executeSql(query); | ||
List<Row> firstResults = CollectionUtil.iteratorToList(firstResult.collect()); | ||
String firstExpected = "+I[1, 1]\n+I[2, 2]\n+I[3, 3]\n"; | ||
TestBaseUtils.compareResultAsText(firstResults, firstExpected); | ||
|
||
TimeUnit.SECONDS.sleep(6); | ||
|
||
TableResult lastResult = tEnv.executeSql(query); | ||
List<Row> lastResults = CollectionUtil.iteratorToList(lastResult.collect()); | ||
assertThat(lastResults).isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same for the hbase-1.4 test)
I think it would worth to have another read at a time where there is still some records in the table, e.g.:
TableResult tRes = tEnv.executeSql(query);
List<Row> actualRows = CollectionUtil.iteratorToList(tRes.collect());
String expectedRows = "+I[1, 1]\n+I[2, 2]\n+I[3, 3]\n";
TestBaseUtils.compareResultAsText(actualRows, expectedRows);
Thread.sleep(4200);
tRes = tEnv.executeSql(query);
actualRows = CollectionUtil.iteratorToList(tRes.collect());
expectedRows = "+I[3, 3]";
TestBaseUtils.compareResultAsText(actualRows, expectedRows);
Thread.sleep(2000);
tRes = tEnv.executeSql(query);
actualRows = CollectionUtil.iteratorToList(tRes.collect());
assertThat(actualRows).isEmpty();
cf8bc1d
to
e5e93bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this feature, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What is the purpose of the change
Support the hbase writable metadata ttl, allow user to set the cell ttl.
Brief change log
Verifying this change
This change is already covered by existing tests, such as