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

Make time-related variables more readable #6158

Merged
merged 3 commits into from Aug 21, 2018

Conversation

Projects
None yet
5 participants
@asdf2014
Contributor

asdf2014 commented Aug 12, 2018

Make time-related variables more readable

@asdf2014

This comment has been minimized.

Show comment
Hide comment
@asdf2014

asdf2014 Aug 12, 2018

Contributor

It seems that the failure of those jobs is not related to this PR. Can someone please help me to restart them? (job#415047952 / job#415047956)

Contributor

asdf2014 commented Aug 12, 2018

It seems that the failure of those jobs is not related to this PR. Can someone please help me to restart them? (job#415047952 / job#415047956)

@fjy fjy added this to the 0.13.0 milestone Aug 13, 2018

Show outdated Hide outdated ...ain/java/io/druid/emitter/ambari/metrics/AmbariMetricsEmitterConfig.java
Show outdated Hide outdated ...itter/src/main/java/io/druid/emitter/graphite/GraphiteEmitterConfig.java
@@ -72,7 +73,7 @@
private final List<PartitionConsumerWorker> consumerWorkers = new CopyOnWriteArrayList<>();
private static final int DEFAULT_QUEUE_BUFFER_LENGTH = 20000;
private static final int CONSUMER_FETCH_TIMEOUT = 10000;
private static final int CONSUMER_FETCH_TIMEOUT_MILLIS = (int) TimeUnit.SECONDS.toMillis(10);

This comment has been minimized.

@jihoonson

jihoonson Aug 14, 2018

Contributor

long makes more sense to me. Would you please change the type as well?

@jihoonson

jihoonson Aug 14, 2018

Contributor

long makes more sense to me. Would you please change the type as well?

This comment has been minimized.

@asdf2014

asdf2014 Aug 16, 2018

Contributor

Yes, I have tried before. But, io.druid.firehose.kafka.KafkaSimpleConsumer#fetch method needs an int type timeoutMs param.

@asdf2014

asdf2014 Aug 16, 2018

Contributor

Yes, I have tried before. But, io.druid.firehose.kafka.KafkaSimpleConsumer#fetch method needs an int type timeoutMs param.

@@ -74,10 +75,10 @@
private List<HostAndPort> replicaBrokers;
private SimpleConsumer consumer = null;
private static final int SO_TIMEOUT = 30000;
private static final int SO_TIMEOUT_MILLIS = (int) TimeUnit.SECONDS.toMillis(30);

This comment has been minimized.

@jihoonson

jihoonson Aug 14, 2018

Contributor

Same here. Can be long.

@jihoonson

jihoonson Aug 14, 2018

Contributor

Same here. Can be long.

This comment has been minimized.

@asdf2014

asdf2014 Aug 16, 2018

Contributor

Yes, I have tried it before. But, the constructor of SimpleConsumer needs an int type soTimeout parameter.

@asdf2014

asdf2014 Aug 16, 2018

Contributor

Yes, I have tried it before. But, the constructor of SimpleConsumer needs an int type soTimeout parameter.

@@ -38,7 +38,7 @@
public class AsyncQueryRunnerTest
{
private static final long TEST_TIMEOUT = 60000;
private static final long TEST_TIMEOUT_MILLIS = 60_000;

This comment has been minimized.

@jihoonson

jihoonson Aug 14, 2018

Contributor

TimeUnit.SECONDS.toMillis(60);?

@jihoonson

jihoonson Aug 14, 2018

Contributor

TimeUnit.SECONDS.toMillis(60);?

This comment has been minimized.

@asdf2014

asdf2014 Aug 16, 2018

Contributor

Yep, I have tried this before too. However, the @Test annotation requires a constant attribute value.

@asdf2014

asdf2014 Aug 16, 2018

Contributor

Yep, I have tried this before too. However, the @Test annotation requires a constant attribute value.

@jihoonson

This comment has been minimized.

Show comment
Hide comment
@jihoonson

jihoonson Aug 14, 2018

Contributor

@asdf2014 thanks! I've left some comments and restarted the failed jobs.

Contributor

jihoonson commented Aug 14, 2018

@asdf2014 thanks! I've left some comments and restarted the failed jobs.

@clintropolis

This comment has been minimized.

Show comment
Hide comment
@clintropolis

clintropolis Aug 14, 2018

Contributor

Nice! 👍 after changes @jihoonson suggested

Contributor

clintropolis commented Aug 14, 2018

Nice! 👍 after changes @jihoonson suggested

@asdf2014

This comment has been minimized.

Show comment
Hide comment
@asdf2014

asdf2014 Aug 16, 2018

Contributor

Hi, @jihoonson . Thanks for your comments. I removed unnecessary boxing of Long type variables. Since some methods / constructors / annotations declare the type of the parameter, I cannot patch other suggestions.

Contributor

asdf2014 commented Aug 16, 2018

Hi, @jihoonson . Thanks for your comments. I removed unnecessary boxing of Long type variables. Since some methods / constructors / annotations declare the type of the parameter, I cannot patch other suggestions.

@jon-wei jon-wei merged commit 3647d4c into apache:master Aug 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asdf2014 asdf2014 deleted the asdf2014:time_variable_readability branch Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment