Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6118: Multi Tenant Workloads using PHERF - #878
Merged
Conversation
jpisaac
commented
Sep 11, 2020
Contributor
- Added configuration classes and interfaces for multi-tenant workloads
jpisaac
commented
Sep 11, 2020
ContributorAuthor
@ChinmaySKulkarni@yanxinyi@gokceni I am breaking this PR into multiple commits so that it is easy to review. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ChinmaySKulkarni
left a comment
Contributor
There was a problem hiding this comment.
Had a quick glance and looks good overall. Is it helpful to add interfaces for some of the configuration classes instead of directly adding solid implementations?
Uh oh!
There was an error while loading. Please reload this page.
| import javax.xml.bind.annotation.XmlType; | ||
| @XmlType | ||
| public class Noop { |
Contributor
There was a problem hiding this comment.
Why do we need this NoOp class for?
Contributor
There was a problem hiding this comment.
I think this is for no operation that simulates the idle time.
ContributorAuthor
There was a problem hiding this comment.
@ChinmaySKulkarni This holds the idle time to be used for waiting. Modeled it as an operation, thus follows the same pattern as other operations.
Contributor
There was a problem hiding this comment.
Can we rename it so it reflects an operation aimed at injecting "idle/wait time"?
Contributor
There was a problem hiding this comment.
Ping @jpisaac I think we should still consider renaming the class so it is clear that it is introduced for the sole purpose of adding wait time. Maybe call it IdleOp
| import java.util.List; | ||
| @XmlType | ||
| public class LoadProfile { |
Contributor
There was a problem hiding this comment.
Can you add header comments for all newly introduced classes?
ContributorAuthor
There was a problem hiding this comment.
@ChinmaySKulkarni added the headers, since you commented let me know if I missed anything.
Contributor
There was a problem hiding this comment.
I meant class-level comments for all the new classes
| .getType() == DataTypeMapping.VARCHAR) ? "'" : ""; | ||
| ret = ret.replace("[" + dynamicField + "]", | ||
| needQuotes + ruleApplier.getDataValue(dynamicColumn).getValue() + needQuotes); | ||
| } |
| @@ -0,0 +1,362 @@ | |||
| <?xml version="1.0" encoding="UTF-8" standalone="yes"?> | |||
Contributor
There was a problem hiding this comment.
I prefer to put this scenario file under /phoenix-pherf/src/main/resources/scenario/ dir since this is an example of how to run instead of a test case.
ContributorAuthor
There was a problem hiding this comment.
@yanxinyi This file is used in testWorkloadWithLoadProfile in ConfigurationParserTest. So I think it makes sense to keep it in the test/resources folder.
jpisaac
commented
Sep 17, 2020
ContributorAuthor
@ChinmaySKulkarni All configuration classes are mapped to XML files (configs/definitions) so making them interfaces may not help and not add much value. Since they typically will have only getters and setters and there will be only one concrete implementation which matches the underlying XML structure. |
stoty
commented
Sep 28, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 19, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Dec 3, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
yanxinyi
reviewed
Dec 8, 2020
| public final ExpectedSystemExit exit = ExpectedSystemExit.none(); | ||
| @Test | ||
| @Ignore |
yanxinyi
reviewed
Dec 8, 2020
Uh oh!
There was an error while loading. Please reload this page.
yanxinyi
reviewed
Dec 8, 2020
Uh oh!
There was an error while loading. Please reload this page.
yanxinyi
reviewed
Dec 8, 2020
| // Assume the first rule map | ||
| Map<DataTypeMapping, List> ruleMap = modelList.get(0); | ||
| List<Column> ruleList = ruleMap.get(phxMetaColumn.getType()); | ||
| //LOGGER.info(String.format("Did not found a correct override column rule, %s, %s", phxMetaColumn.getName(), phxMetaColumn.getType())); |
yanxinyi
reviewed
Dec 8, 2020
| statement.setNull(count, Types.DATE); | ||
| } else { | ||
| Date | ||
| date = |
Contributor
There was a problem hiding this comment.
can you address this in the next PR
yanxinyi
reviewed
Dec 8, 2020
Uh oh!
There was an error while loading. Please reload this page.
yanxinyi
reviewed
Dec 8, 2020
Uh oh!
There was an error while loading. Please reload this page.
jpisaacforce-pushed
the
PHOENIX-6118-PR
branch
from
December 10, 2020 23:55
1086c20 to
ede0f62Comparestoty
commented
Dec 11, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
| ( | ||
| GID | ||
| ) | ||
| ) AS SELECT * FROM PHERF.TEST_MULTI_TENANT_TABLE WHERE IDENTIFIER = 'EV1' |
Contributor
There was a problem hiding this comment.
I didn't find the DDL for PHERF.TEST_MULTI_TENANT_TABLE
ChinmaySKulkarni
left a comment
Contributor
There was a problem hiding this comment.
Added some comments @jpisaac
| public final ExpectedSystemExit exit = ExpectedSystemExit.none(); | ||
| @Test | ||
| @Ignore |
| for (Scenario scenario : model.getScenarios()) { | ||
| LOGGER.debug(String.format("Testing %s", scenario.getName())); | ||
| LoadProfile loadProfile = scenario.getLoadProfile(); | ||
| assertTrue("tenant group size is not as expected: ", |
Contributor
There was a problem hiding this comment.
nit: Use assertEquals() instead
Contributor
There was a problem hiding this comment.
Same for other such instances
| while (ops-- > 0) { | ||
| TenantOperationInfo info = evtGen.next(); | ||
| TenantOperationImpl op = opFactory.getOperation(info); | ||
| int row = TestOperationGroup.valueOf(info.getOperationGroupId()).ordinal(); |
Contributor
There was a problem hiding this comment.
can't we just use the enum value instead of referring to its ordinal here?
| public class MultiTenantOperationBaseIT extends ParallelStatsDisabledIT { | ||
| static enum TestOperationGroup { | ||
| op1, op2, op3, op4, op5 |
Contributor
There was a problem hiding this comment.
There seems to be some inherent assumption what each operation group does i.e. upsert vs NoOp, etc. as per my understanding. Can you rename the enum values and/or add some comments to clarify this?
| LOGGER.info(pUtil.getGSON().toJson(stats)); | ||
| if (info.getOperation().getType() == Operation.OperationType.PRE_RUN) continue; | ||
| switch (row) { | ||
| case 0: |
Contributor
There was a problem hiding this comment.
Why not use switch on the enum values themselves rather than the ordinal?
| /** | ||
| * Holds metrics + contextual info on the operation run. | ||
| */ | ||
| public class OperationStats { |
Contributor
There was a problem hiding this comment.
Shouldn't operationStats really be different per Operation? Operation is an interface whereas this is a concrete class. Each operation type might have their own stats, no? Maybe make this an abstract class instead and have each operation type implement their own stats which extend this?
| long duration = EnvironmentEdgeManager.currentTimeMillis() - startTime; | ||
| return new OperationStats(input, startTime, 0, 0, duration); | ||
| } catch (InterruptedException e) { | ||
| e.printStackTrace(); |
| Scenario testScenarioWithLoadProfile = scenarioList.get(0); | ||
| LoadProfile loadProfile = testScenarioWithLoadProfile.getLoadProfile(); | ||
| assertTrue("batch size not as expected: ", |
Contributor
There was a problem hiding this comment.
Nit: Use assertEquals() everywhere
| // If we don't parse the DTD, the variable 'name' won't be defined in the XML | ||
| LOGGER.warn("Caught expected exception", e); | ||
| } | ||
| return null; |
Contributor
There was a problem hiding this comment.
Are we handling this null value in the caller?
| for (Scenario scenario : model.getScenarios()) { | ||
| LOGGER.debug(String.format("Testing %s", scenario.getName())); | ||
| LoadProfile loadProfile = scenario.getLoadProfile(); | ||
| assertTrue("tenant group size is not as expected: ", |
Contributor
There was a problem hiding this comment.
nit: assertEquals throughout
stoty
commented
Jan 6, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 9, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
yanxinyi
commented
Jan 27, 2021
Contributor
@jpisaac can you handle the above comments and solve the conflicts? |
jpisaacforce-pushed
the
PHOENIX-6118-PR
branch
from
February 26, 2021 22:13
715efd9 to
5b4be38Comparejpisaac
commented
Feb 26, 2021
ContributorAuthor
@ChinmaySKulkarni@yanxinyi Rebased it to 4.x tip |
stoty
commented
Feb 26, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
added 6 commits
March 23, 2021 16:39
stoty
commented
Mar 24, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
jpisaac
commented
Mar 24, 2021
ContributorAuthor
@yanxinyi@ChinmaySKulkarni for 4.x |
stoty
commented
Mar 24, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
| import org.apache.phoenix.pherf.result.ResultValue; | ||
| import org.apache.phoenix.pherf.result.file.ResultFileDetails; | ||
| import org.apache.phoenix.pherf.result.impl.CSVFileResultHandler; | ||
| import org.junit.Ignore; |
Contributor
There was a problem hiding this comment.
nit: can you remove this unused import from the next PR. Don't need to fix it now
yanxinyi
approved these changes
Mar 26, 2021
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
4 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.