Skip to content
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

Improve `collection` related things that reusing a immutable object instead of creating a new object #4135

Merged
merged 1 commit into from May 16, 2017

Conversation

Projects
None yet
4 participants
@asdf2014
Copy link
Member

asdf2014 commented Mar 30, 2017

Improve collection related things that reusing a immutable object instead of creating a new object

  • Collections.singletonList(something) is immutable whereas Arrays.asList(something) is a fixed size List representation of an Array where the List and Array gets joined in the heap.
  • Collections.emptyList() reuses an object instead of creating a new object as it will be the case with Arrays.asList().
return Arrays.asList(
new SimpleModule("ParuqetInputRowParserModule")
return Collections.singletonList(
new SimpleModule("ParquetInputRowParserModule")

This comment has been minimized.

@leventov

leventov Mar 30, 2017

Member

Is it fully compatible? Couldn't somebody depend on the actual name of the module somehow?

This comment has been minimized.

@asdf2014

asdf2014 Mar 31, 2017

Author Member

I think just we can ensure the name of SimpleModule be unique, that will be okay.

@@ -113,7 +113,8 @@ public IntervalChunkingQueryRunner(
),
executor, queryWatcher
).run(
query.withQuerySegmentSpec(new MultipleIntervalSegmentSpec(Arrays.asList(singleInterval))),
query.withQuerySegmentSpec(new MultipleIntervalSegmentSpec(
Collections.singletonList(singleInterval))),

This comment has been minimized.

@leventov

leventov Mar 30, 2017

Member

Last two )) should be on the next line

This comment has been minimized.

@asdf2014

asdf2014 Mar 31, 2017

Author Member

I got it.

@@ -71,7 +71,7 @@ public boolean eval(Row row)
public byte[] getCacheKey()
{
final byte[] aggBytes = StringUtils.toUtf8(aggregationName);
final byte[] valBytes = Bytes.toArray(Arrays.asList(value));
final byte[] valBytes = Bytes.toArray(Collections.singletonList(value));

This comment has been minimized.

@leventov

leventov Mar 30, 2017

Member

Could be new byte[] {value.byteValue()}

This comment has been minimized.

@asdf2014

asdf2014 Mar 31, 2017

Author Member

Great!

@@ -138,12 +138,12 @@ public static CompressedVSizeIndexedSupplier fromIterable(


@Override
public IndexedMultivalue<IndexedInts> get()
public IndexedMultiValue<IndexedInts> get()

This comment has been minimized.

@leventov

leventov Mar 30, 2017

Member

I don't think it should be renamed

This comment has been minimized.

@asdf2014

asdf2014 Mar 31, 2017

Author Member

U r right. I'll roll it back.

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented Mar 31, 2017

Timeout Problem happened again... Somebody could help me to rerun the Travis job? :(

java.lang.Exception: test timed out after 60000 milliseconds
	at java.lang.Thread.sleep(Native Method)
	at io.druid.server.coordinator.DruidCoordinatorTest.testCoordinatorRun(DruidCoordinatorTest.java:375)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:483)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74)
@jihoonson

This comment has been minimized.

Copy link
Contributor

jihoonson commented Mar 31, 2017

LGTM. Thanks @asdf2014!

@leventov

This comment has been minimized.

Copy link
Member

leventov commented May 16, 2017

@asdf2014 could you please resolve conflicts? Then we can merge this PR.

@leventov

This comment has been minimized.

Copy link
Member

leventov commented May 16, 2017

@asdf2014 your latest commit doesn't have druid-io's master HEAD commit as parent, so Github doesn't hide repetitive changes. Could you please add parent to your latest commit (http://stackoverflow.com/a/4264151/648955 might help) and re-push it?

@leventov

This comment has been minimized.

Copy link
Member

leventov commented May 16, 2017

In the future please resolve conflicts via git merge master, as described here: https://github.com/druid-io/druid/blob/master/CONTRIBUTING.md#github-workflow

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented May 16, 2017

@leventov Hi, Leventov. I executed the git rebase --root --onto master --preserve-merges command on code_refactoring branch and get the Successfully rebased and updated refs/heads/code_refactoring. message. Is that ok?

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented May 16, 2017

If that is fine. I would push it with the git push -u origin code_refactoring --force command.

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented May 16, 2017

@leventov Already Done.

git rebase --abort
git checkout code_refactoring
git checkout -b re2
git branch root bbb61e638b391d29
git checkout re2
git diff master > ../123.patch
git checkout master
git checkout -b r2
git apply ../123.patch
git diff master
git status
git diff --name-status | wc -l
git add .
git status
git diff master
git commit -m 'Improve `collection` related things that reusing a immutable object instead of creating a new object'
git status
git push origin r2:code_refactoring -f
@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented May 16, 2017

Why i changed nothing, but i got the The job exceeded the maximum time limit for jobs, and has been terminated. message from travis? It seems like the time length limitation of the time of build job is 50min in travis.org. (https://docs.travis-ci.com/user/customizing-the-build#Build-Timeouts)

Can anyone help me to restart the build job?

@gianm

gianm approved these changes May 16, 2017

@gianm

This comment has been minimized.

Copy link
Contributor

gianm commented May 16, 2017

@asdf2014 looks like the tests have passed now. thanks for the contribution!

@gianm gianm added the Improvement label May 16, 2017

@gianm gianm added this to the 0.10.1 milestone May 16, 2017

@gianm gianm merged commit e823085 into apache:master May 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -80,14 +80,12 @@ public GenericRecord parse(ByteBuffer bytes)
DatumReader<GenericRecord> reader = new GenericDatumReader<>(schema);
try (ByteBufferInputStream inputStream = new ByteBufferInputStream(Collections.singletonList(bytes))) {
return reader.read(null, DecoderFactory.get().binaryDecoder(inputStream, null));
}
catch (EOFException eof) {
} catch (EOFException eof) {

This comment has been minimized.

@leventov

leventov May 16, 2017

Member

Actually this violate Druid code style. Former version of code is correct

@leventov

This comment has been minimized.

Copy link
Member

leventov commented May 16, 2017

@asdf2014 irrelevant now but actually what you did is rebase of your entire branch, that is also should be avoided. I asked to rebase only the latest commit in the branch, which you was proposing to merge into druid-io/druid/master, asdf2014:code_refactoring. That commit used to be called "Fix conflicts".

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented May 17, 2017

@leventov Sorry violate druid code style... I got it, in fact i also liked squash those commits into a single commit. @gianm You are welcome.

@leventov

This comment has been minimized.

Copy link
Member

leventov commented May 17, 2017

@asdf2014 for the future PRs - please don't squash anything after you submitted a PR and some time has passed so somebody may has already started looking at your PR (and, moreover, if somebody already left some review comments), because reviewers lose the progress and in some cases need to re-review the whole PR, to be sure that they didn't miss anything.

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented May 17, 2017

Okay, you are right. Those messages of commits should be retained.

@gianm

This comment has been minimized.

Copy link
Contributor

gianm commented May 17, 2017

In addition to the messages, Github also has a nice feature that lets reviewers see what has changed since their last review. It's based on diffing against new commits though, so squashing prevents that feature from working.

FWIW we only very recently changed our guidelines to suggest that people avoid squashing (#4087). Until then we did suggest squashing, but we changed our minds :)

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented May 17, 2017

Oh... Don't know these, i will pay attention in the future.

@gianm

This comment has been minimized.

Copy link
Contributor

gianm commented May 17, 2017

No worries, and thanks again for the patch.

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented May 17, 2017

Alright, you are welcome.

@asdf2014 asdf2014 deleted the asdf2014:code_refactoring branch Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.