-
Notifications
You must be signed in to change notification settings - Fork 135
IGNITE-15492 Check schema availability on local node #344
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
Conversation
b06f486 to
4669592
Compare
| @NotNull ConfigurationNotificationEvent<SchemaView> schemasCtx) { | ||
| try { | ||
| ((SchemaRegistryImpl)tables.get(ctx.newValue().name()).schemaRegistry()). | ||
| ((SchemaRegistryImpl)tables.get(ctx.newValue().name()).schemaView()). |
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.
Could you explain removing schemaRegistry method from AbstractTableView and replacing it by schemaView in TableImpl, which returns SchemaRegistrydespite of the name?
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.
Because the schemaView method is weightily used in our code before.
The one reason by that another method (schemaRegistry) was written is Alexander did not know about it already exists.
4669592 to
b0e88ae
Compare
e56c223 to
6a6f67f
Compare
6a6f67f to
89a8476
Compare
| schemaReg = new SchemaRegistryImpl(0, (v) -> schemaDesc); | ||
|
|
||
| schemaReg = new SchemaRegistryImpl(0, (v) -> schemaDesc, () -> INITIAL_SCHEMA_VERSION); |
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.
We have SchemaRegistryImpl.INITIAL_SCHEMA_VERSION = -1 and TableManager.INITIAL_SCHEMA_VERSION = 1 - is it ok?
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.
Both of these constant were added later than my patch.
I think not, but it is not my war).
...ner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItDataSchemaSyncTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| assertNotNull(metaMngr); | ||
|
|
||
| WatchAggregator aggregator = (WatchAggregator) ReflectionUtils.tryToReadFieldValue( |
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.
Have we any plans and jira issues to remove this dirty reflection/mocking-based approach?
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.
We have the issue IGNITE-15723 to resolve this reference without any reflection mechanism (it is written couple line upper).
But I only move this code to use it into various tests, but the code of Inhibitor have been existing for a month.
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaRegistry.java
Outdated
Show resolved
Hide resolved
453810e to
3f11ae5
Compare
No description provided.