Kafka Hardlimit by xiaoyao1991 · Pull Request #102 · twitter-forks/presto

xiaoyao1991 · 2017-08-14T22:48:57Z

No description provided.

Upgrading to 0.132-tw-0.16

Upgrade to 0.139-tw-17

…scovery Add a few logs in DiscoveryNodeManager to debug 'No worker nodes' error.

…_queries Add query logging of all presto queries

Upgrade to 0.139-tw-0.18

…sitive_parquet_column_match Look up parquet columns by name case-insensitive

…to_139_19

…are_parquet_column_match Handle hive keywords when doing a name-based parquet field lookup

Upgrade to 0.139-tw-0.19

[maven-release-plugin] copy for tag 0.141 # Conflicts: # pom.xml

Upgrade to 0.141-tw-0.20

upgrade Presto to 0.179

…at-off-in-default Set default value for hive statistic feature to false

…function signature

Upgrade to 181

Thrift decoder

Kafka07 plugin

Fix build and style

…iver-node-version Tableau Presto ODBC Driver fix

xiaoyao1991 · 2017-08-14T22:51:25Z

@maosongfu

maosongfu · 2017-08-15T03:11:39Z

@@ -76,7 +76,17 @@ public class KafkaConnectorConfig
    /**
     * Fetch size


Can you elaborate more about what Fetch Size is

maosongfu · 2017-08-15T03:13:19Z

+    private DataSize fetchSize = new DataSize(10, Unit.MEGABYTE);
+
+    /**
+     * Default Query Interval


This is a variable. So:

Don't call it defaultXXXX.

Default value should be a final value. Here, it is good to have one final Duration value called DEFAULT_QUERY_RANGE and a variable called queryInterval. And then assign the default value of the variable with the constant value.

Also, can you elaborate more about what this variable means? I have to read through the logic code using this variable to understand it.

I think based on the code style in Presto, it's better just assign the default value here. I agree that the variable name here is a little bit confusing. But the "defaultQueryInterval" means that this value is the default query interval if the query interval is not specified in SQL.

Basing on the code logic, i.e. this value can be assigned. It is not a default value. Default value is a constant value.

defaultQueryInterval is the variable and Duration.valueOf("10m") is the default value for this variable which is a constant value.

As above:

This is a variable. So:

Don't call it defaultXXXX.

Default value should be a final value. Here, it is good to have one final Duration value called DEFAULT_QUERY_RANGE and a variable called queryInterval. And then assign the default value of the variable with the constant value.

Call it queryInterval if it is a variable. (Or queryRange can be more descriptive). Otherwise, it is confusing; people need to go through the code logic to understand it.

As above:

But the "defaultQueryInterval" means that this value is the default query interval if the query interval is not specified in SQL.

maosongfu · 2017-08-15T03:14:50Z

+    private Duration defaultQueryInterval = Duration.valueOf("10m");
+
+    /**
+     * Hard limit on


Also, can you elaborate more about what this variable means? I have to read through the logic code using this variable to understand it.

maosongfu · 2017-08-15T03:14:59Z

+    /**
+     * Hard limit on
+     */
+    private boolean hardLimitOn = true;


isHardLimitOn

No. "isHardLimitOn" should be a method instead of a variable.

Then the variable name could be hardLimited

maosongfu · 2017-08-15T03:17:09Z


    @Config("kafka.fetch-size")
-    public KafkaConnectorConfig setFetchSize(int fetchSize)
+    public KafkaConnectorConfig setFetchSize(String fetchSize)


I have a feeling you want to use the Builder pattern to achieve the immutable Config class. But it seems self-finished; not that great.

No, this class is reading the config files

maosongfu · 2017-08-15T03:28:47Z

        }

        log.info("startTs: %s, endTs: %s", startTs, endTs);
+        if (config.isHardLimitOn() && startTs == null && endTs == null) {


I don't understand how the hard_limit is applied. Can you add more comments to make it clear? Especially for the for (Map.Entry<ColumnHandle, Domain> entry : columnHandleDomainMap.entrySet()) loop above.

Also, could we check whether it meets the expectations:

If users specify a time range exceeding the limitation, an exception should be thrown directly. Otherwise, the behavior is changed silently and unexpectedly.

If users do not specify the range, we could either throw an exception directly, or trim to the limited time range with an explicit logging/print-out notifying users.

Here are some thoughts about your expectations:

If the user doesn't know how to specify a range, they will fall into default query interval.

If the user knows how to specify a range, they must agree with default query interval and must have a specific reason to go beyond the default query interval. It would be costly to serve the users if they actually have this specific request. It will involve code review and service deploy processes which wouldn't the ideal way for serving our customers for urgent needs.

If the user doesn't know how to specify a range, Presto should either throw an exception directly, or trim to the limited time range with an explicit logging/print-out notifying users. Silently unexpected behavior is bad.

A design should be robust to users' mistakes/errors, rather than to rely on users always providing safe input. Also, this is what limitation means: it should not happen.

If users want to query exceeding the limitation, they need to notify all stakeholders, for instance, messaging team, since it could potentially break the whole kafka service, leading to a SEV0 or SEV1.

maosongfu · 2017-08-15T03:29:20Z

    {
        return testSessionBuilder()
-                .setCatalog("kafka")
+                .setCatalog("kafka07")


This is good. I like the version tag attached.

Yaliang · 2017-08-15T04:17:47Z

+    private DataSize fetchSize = new DataSize(10, Unit.MEGABYTE);
+
+    /**
+     * Default Query Interval


I think based on the code style in Presto, it's better just assign the default value here. I agree that the variable name here is a little bit confusing. But the "defaultQueryInterval" means that this value is the default query interval if the query interval is not specified in SQL.

Yaliang · 2017-08-15T04:19:24Z


    @Config("kafka.fetch-size")
-    public KafkaConnectorConfig setFetchSize(int fetchSize)
+    public KafkaConnectorConfig setFetchSize(String fetchSize)


No, this class is reading the config files

Yaliang · 2017-08-15T04:20:52Z

+    /**
+     * Hard limit on
+     */
+    private boolean hardLimitOn = true;


No. "isHardLimitOn" should be a method instead of a variable.

Yaliang · 2017-08-15T04:33:58Z

        }

        log.info("startTs: %s, endTs: %s", startTs, endTs);
+        if (config.isHardLimitOn() && startTs == null && endTs == null) {


Here are some thoughts about your expectations:

If the user doesn't know how to specify a range, they will fall into default query interval.

If the user knows how to specify a range, they must agree with default query interval and must have a specific reason to go beyond the default query interval. It would be costly to serve the users if they actually have this specific request. It will involve code review and service deploy processes which wouldn't the ideal way for serving our customers for urgent needs.

Bill Graham and others added 30 commits February 11, 2016 15:40

Upgrading to 0.132-tw-0.16

daa3773

Merge pull request twitter-forks#26 from billonahill/upgrade_to_132_16

b71195c

Upgrading to 0.132-tw-0.16

Change version to 0.139 to not conflict with merge

f4fed25

merge from 0.139

b3a94e1

upgrade pom version to 0.139-tw-0.17

2c9da60

Merge pull request twitter-forks#27 from billonahill/upgrade_to_139_17

d295eea

Upgrade to 0.139-tw-17

Adding query logging to presto queries

582df38

Add a few logs in DiscoveryNodeManager to debug 'No worker nodes' error.

04e60cc

Merge pull request twitter-forks#29 from saileshmittal/add-logs-to-di…

56582c3

…scovery Add a few logs in DiscoveryNodeManager to debug 'No worker nodes' error.

Merge pull request twitter-forks#28 from billonahill/billg/log_presto…

280e37c

…_queries Add query logging of all presto queries

Upgrade to 0.139-tw-0.18

4ca01a7

Merge pull request twitter-forks#30 from saileshmittal/upgrade_to_139_18

3a4a156

Upgrade to 0.139-tw-0.18

Look up parquet columns by name case-insensitive

f1085e0

Merge pull request twitter-forks#31 from billonahill/billg/case_insen…

775eee8

…sitive_parquet_column_match Look up parquet columns by name case-insensitive

Upgrading to 0.139-tw-0.19

5b4647a

Handle hive keywords when doing a name-based parquet field lookup

f766a0c

Merge branch 'billg/keyword_aware_parquet_column_match' into upgrade_…

a062c56

…to_139_19

Refactor to make diff cleaner

ca9961e

Merge branch 'billg/keyword_aware_parquet_column_match' into upgrade_…

423f5cf

…to_139_19

Merge pull request twitter-forks#32 from billonahill/billg/keyword_aw…

a1807ed

…are_parquet_column_match Handle hive keywords when doing a name-based parquet field lookup

Merge branch 'twitter-master' into upgrade_to_139_19

d687155

Merge pull request twitter-forks#33 from billonahill/upgrade_to_139_19

7eb8909

Upgrade to 0.139-tw-0.19

Upgrade to 0.141.

d7d66a2

Merge tag '0.141' into upgrade_to_141_20

a7a5233

[maven-release-plugin] copy for tag 0.141 # Conflicts: # pom.xml

Upgrade to 0.141-tw-0.20.

94bcf3e

Merge pull request twitter-forks#34 from saileshmittal/upgrade_to_141_20

dc6bebe

Upgrade to 0.141-tw-0.20

Use modules and query events for logging

3de1ca4

Add javadocs

f055db6

Add javadocs

3c1c7b6

fix imports

d8afd62

Yaliang and others added 22 commits June 18, 2017 22:54

prepare release 0.179-tw-0.36

20c2816

Fit new event

a7f633f

Fit temp path change for tests

c517f40

Merge pull request twitter-forks#90 from Yaliang/yaliangw/upgrade-0.179

f2a066d

upgrade Presto to 0.179

Set default value for hive statistic feature to false

2f85330

Merge pull request twitter-forks#92 from Yaliang/yaliangw/set-hive-st…

d794e7c

…at-off-in-default Set default value for hive statistic feature to false

Thrift Decoder

77ded78

cleanup

d98b04e

avoid unnecessary version conflict during merge

d8a6e9e

merged 0.181, resolved conflicts

791d3d5

use twitter tag 0.181-tw-0.37, update ThriftHiveRecordCursorProvider …

c8a76bb

…function signature

Merge pull request twitter-forks#98 from dabaitu/upgrade_to_181

70baf50

Upgrade to 181

Merge pull request twitter-forks#95 from xiaoyao1991/thrift_decoder_fin

92a9ebc

Thrift decoder

clone presto-kafka as presto-kafka07 plugin

1936564

kafka07 changes

47ac252

address comments

a5f13ce

Merge pull request twitter-forks#96 from xiaoyao1991/kafka07_plugin

ab45afb

Kafka07 plugin

Fix build and style

5feb582

Merge pull request twitter-forks#99 from xiaoyao1991/fix_build

dadbfe0

Fix build and style

Tableau Presto ODBC Driver fix

8725ff9

Merge pull request twitter-forks#101 from Yaliang/yaliangw/tableau-dr…

266ec2c

…iver-node-version Tableau Presto ODBC Driver fix

init

4e23932

xiaoyao1991 changed the title ~~init~~ Kafka Hardlimit Aug 14, 2017

cleanup

1ac12be

better configs

2d233c2

Yaliang approved these changes Aug 14, 2017

View reviewed changes

maosongfu reviewed Aug 15, 2017

View reviewed changes

Yaliang approved these changes Aug 15, 2017

View reviewed changes

beinan force-pushed the twitter-master branch from b51d979 to 9a609f3 Compare August 24, 2020 06:45

		@@ -76,7 +76,17 @@ public class KafkaConnectorConfig
		/**
		* Fetch size

Conversation

xiaoyao1991 commented Aug 14, 2017

Uh oh!

xiaoyao1991 commented Aug 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maosongfu Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maosongfu Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yaliang Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maosongfu Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yaliang Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maosongfu Aug 15, 2017 •

edited

Loading

maosongfu Aug 15, 2017 •

edited

Loading

Yaliang Aug 15, 2017 •

edited

Loading

maosongfu Aug 15, 2017 •

edited

Loading

Yaliang Aug 15, 2017 •

edited

Loading