Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6171 Child views should not be allowed to override the parent view PHOENIX_TTL attribute. - #916
Closed
jpisaac wants to merge 3 commits into
Closed
PHOENIX-6171 Child views should not be allowed to override the parent view PHOENIX_TTL attribute.#916jpisaac wants to merge 3 commits into
jpisaac wants to merge 3 commits into
Conversation
jpisaac
commented
Oct 9, 2020
ContributorAuthor
stoty
commented
Oct 9, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
| PTable table = getTableFromCache(cacheKey, clientTimeStamp, clientVersion); | ||
| // Disallow if the parent has PHOENIX_TTL set. | ||
| if (parentTable != null && parentTable.getPhoenixTTL() != PHOENIX_TTL_NOT_DEFINED) { |
Contributor
There was a problem hiding this comment.
just want to confirm that we don't support more than 2 level case. For example, we have a global view V1 of the TTL set. The V2 is a view on top of the V1. If we create a new V3 on of the v2 with a TTL value, this won't catch and allow the V3 view to have a TTL value, which might conflict with V1 TTL.
better to document somewhere to notice other ppl
ContributorAuthor
There was a problem hiding this comment.
@yanxinyi Currently the PHOENIX_TTL value is stored with every view and at every level - so in this case V2 would have the same PHOENIX_TTL value as of V1. So when creating V3 the parentTable.getPhoenixTTL() != PHOENIX_TTL_NOT_DEFINED.
But realized as was going thru this that I need an additional check to ensure that the PHOENIX_TTL is being altered
stoty
commented
Oct 17, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
jpisaac
commented
Oct 26, 2020
ContributorAuthor
@yanxinyi@ChinmaySKulkarni rebased and fixed conflicts |
ChinmaySKulkarni
requested changes
Oct 26, 2020
| + "WHERE %s AND TABLE_SCHEM = '%s' AND TABLE_NAME = '%s' AND TABLE_TYPE = '%s'"; | ||
| private static final String | ||
| ALTER_PHOENIX_TTL_SQL = |
Contributor
There was a problem hiding this comment.
nit: Modify formatting to avoid extra line
| "ALTER VIEW \"%s\".\"%s\" set PHOENIX_TTL=%s"; | ||
| private static final String ALTER_PHOENIX_TTL_SQL = "ALTER VIEW %s.%s set PHOENIX_TTL=%d"; | ||
| private static final String |
Contributor
There was a problem hiding this comment.
nit: Modify formatting to avoid extra line
| RowFilter | ||
| schemaNameFilter = | ||
| new RowFilter(CompareFilter.CompareOp.EQUAL, new SubstringComparator(schemaName)); | ||
| QualifierFilter |
Contributor
There was a problem hiding this comment.
Similar here. Can you please check if your IDE has the Phoenix IDE settings?
| final SchemaBuilder schemaBuilder = createLevel2TenantViewWithGlobalLevelTTL(null, null); | ||
| String tenantId = schemaBuilder.getDataOptions().getTenantId(); | ||
| String |
Contributor
There was a problem hiding this comment.
nit: Same formatting issues here potentially.
| String.format(ALTER_SQL_WITH_NO_TTL, schemaName, globalViewName, "COL_30"); | ||
| stmt.execute(sql); | ||
| } | ||
| } catch (SQLException e) { |
Contributor
There was a problem hiding this comment.
This catch block is unnecessary, you can remove it.
| // If a child view exists and the parent previously had a PHOENIX_TTL value set then that | ||
| // implies that the child view too has a valid PHOENIX_TTL (non zero). | ||
| // In this case we do not allow for ALTER of the parent view PHOENIX_TTL value. | ||
| if (!childViews.isEmpty()) { |
Contributor
There was a problem hiding this comment.
What if we're setting the TTL for the first time for a parent view i.e. it was PHOENIX_TTL_NOT_DEFINED first and now we're setting it for the first time ever. This check will disallow that too. Is that intended?
ContributorAuthor
There was a problem hiding this comment.
Will change the comment to be less confusing.
| } | ||
| // Since we do not allow propagation of PHOENIX_TTL values during ALTER for now. | ||
| // If a child view exists and the parent previously had a PHOENIX_TTL value set then that |
Contributor
There was a problem hiding this comment.
We aren't checking the "and the parent previously had a PHOENIX_TTL value set " part. Shouldn't we do that too?
ContributorAuthor
There was a problem hiding this comment.
As discussed offline, changing the comment will bring more clarity. The checks are for "this" view's parent and its children
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| // Checks whether a non-zero PHOENIX_TTL value is being set. | ||
| private boolean settingNewPhoenixTTLAttribute(List<Mutation> tableMetadata, byte[] phoenixTtlBytes) { |
Contributor
There was a problem hiding this comment.
Can you rename this to settingNewPhoenixTTLAttributeForView() to indicate it only applies to views and then also add the check for the same inside the method?
Uh oh!
There was an error while loading. Please reload this page.
stoty
commented
Oct 26, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
added 2 commits
October 26, 2020 20:46
jpisaac
commented
Oct 27, 2020
ContributorAuthor
@ChinmaySKulkarni@yanxinyi Addressed review comments and rebased to resolve merge conflicts. |
stoty
commented
Oct 27, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
ChinmaySKulkarni
approved these changes
Oct 27, 2020
stoty
commented
Oct 28, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Nov 2, 2020
Contributor
JIRA appears to be closed as fixed. Closing PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.