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

indexer designation #114

Merged
merged 8 commits into from
Sep 5, 2019
Merged

indexer designation #114

merged 8 commits into from
Sep 5, 2019

Conversation

houxinli
Copy link
Collaborator

@houxinli houxinli commented Sep 4, 2019

Indexer designation by class name

fixes #113

In the compiler, usage like layer.setIndexerType("PsqlPredicatedTableIndexer") can be used to designate specific indexer for any layer. Backward compatibility is maintained.

I gave up using indexer as fieldname in the compiler because the keyword transient will cause the getIndexer method which is used in boxGetter to return null. I have no idea why this would happen.

Alternatively I introduced the third_party package and the Exclude annotation (see here for more information) to exclude fields during serialization as my discussion with @tracyhenry. I applied this for the FirstRequestHandler, but I'm not sure if it is the only place where this annotation is needed.

I tesed the indexer designation on the predicated table example.

Welcome to comment, discuss, criticise.

@tracyhenry
Copy link
Owner

Thanks for this.

I'm still seeing those boolean variables (isAutoDD, isPredicatedTable, isTable, etc) in Layer.java & Layer.js. We should get rid of them, right?

@houxinli
Copy link
Collaborator Author

houxinli commented Sep 4, 2019

Thanks for this.

I'm still seeing those boolean variables (isAutoDD, isPredicatedTable, isTable, etc) in Layer.java & Layer.js. We should get rid of them, right?

I'm temporarily keeping them to ensure backward compatibility. What is your opinion on this?

@tracyhenry
Copy link
Owner

tracyhenry commented Sep 4, 2019

Thanks for this.
I'm still seeing those boolean variables (isAutoDD, isPredicatedTable, isTable, etc) in Layer.java & Layer.js. We should get rid of them, right?

I'm temporarily keeping them to ensure backward compatibility. What is your opinion on this?

let's get rid of them, backward compatibility isn't necessary at this point before a release 😄

@houxinli
Copy link
Collaborator Author

houxinli commented Sep 4, 2019

Thanks for this.
I'm still seeing those boolean variables (isAutoDD, isPredicatedTable, isTable, etc) in Layer.java & Layer.js. We should get rid of them, right?

I'm temporarily keeping them to ensure backward compatibility. What is your opinion on this?

let's get rid of them, backward compatibility isn't necessary at this point before a release 😄

Sure. My predicated table works fine (if you ignore that MySQL is not supported), but this line (

if (isAutoDDLayer) colListStr += "cluster_num, ";
) seems dependant on isAutoDDLayer, do you have a work around?

@tracyhenry
Copy link
Owner

just check indexerType==AutoDDInMemoryIndexer?

compiler/src/Layer.js Outdated Show resolved Hide resolved
change from console.log to throw new error
@tracyhenry
Copy link
Owner

cleaned up a little bit (see last commit). I'm going to do some testing and then merge

@houxinli
Copy link
Collaborator Author

houxinli commented Sep 5, 2019

cleaned up a little bit (see last commit). I'm going to do some testing and then merge

Sure. Thank you!

@tracyhenry
Copy link
Owner

tracyhenry commented Sep 5, 2019

I replaced try/catch in getIndexerByType with throw after the method signature. This is to prepare for fixing #15.

Ideally we want smaller subroutines to throw exceptions and then catch the exceptions at higher-levels, e.g. when running indexer.createMV or in handle() of request handlers. That way, we avoid subroutines to execute code after the exception (which can be dangerous), and can provide reasonable summary of at what stage (index, online) the exception happened.

Right now, there is almost no try/catch in the back-end. So part of the fix to #15 is to place try/catches in reasonable places.

@houxinli
Copy link
Collaborator Author

houxinli commented Sep 5, 2019

I replaced try/catch in getIndexerByType with throw after the method signature. This is to prepare for fixing #15.

Ideally we want smaller subroutines to throw exceptions and then catch the exceptions at higher-levels, e.g. when running indexer.createMV or in handle() of request handlers. That way, we avoid subroutines to execute code after the exception (which can be dangerous), and can provide reasonable summary of at what stage (index, online) the exception happened.

Right now, there is almost try/catch in the back-end. So part of the fix to #15 is to place try/catches in reasonable places.

This is great! I'll pay more attention to this part

@tracyhenry tracyhenry merged commit c71b576 into master Sep 5, 2019
@tracyhenry tracyhenry deleted the ud-indexer branch September 5, 2019 19:54
ericazhou7 pushed a commit to ericazhou7/Kyrix that referenced this pull request Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Layer designated indexer using names of indexer class
2 participants